Shouldn't _contentLength / setContentLength be factored out?

[prozacgod]

I noticed that there's a

void setContentLength(size_t contentLength) { _contentLength = contentLength; }

Its one and only one use is inside the header in 'streamFile' where the contentLength is available already via file.size()

template<typename T> size_t streamFile(T &file, const String& contentType){
  if (String(file.name()).endsWith(".gz") &&
      contentType != "application/x-gzip" &&
      contentType != "application/octet-stream"){
    sendHeader("Content-Encoding", "gzip");
  }
  send(200, contentType, "", file.size());
  return _currentClient.write(file, HTTP_DOWNLOAD_UNIT_SIZE);
}

So adding an overloaded send that allows a code/content/body/size which I noticed doesn't exist... except for - send_P which is related to PROGMEM / PGM_P (I could be miss understanding this)

It sorta looks like the _contentLength and supporting code is a kludge around missing a send with this footprint. Which I sorta expected to have, maybe the progmen/pgm_p stuff is more common?

void ESP8266WebServer::send(int code, const char* content_type, char* content, size_t size)

Refactoring this would reduce variable clutter and factor out a state check within_prepareHeader that isn't used for any other functionality and add a function that appears to be missing.

---

Unless it's there due to backwards compatibility.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[igrr]

_contentLength is referenced in a bunch of places in ESP8266WebServer.cpp, and is far from being unused.
It is required for two scenarios:

• when sending an amount of data which is not known in advance. In this case Content-Length header should be ommited, which is achieved by calling setContentLength(CONTENT_LENGTH_UNKNOWN). Then one can call send(code, content_type, ""); and send payload using server.sendContent or server.sendContent_P.
• when sending an amount of data which is known in advance, but not available in one contiguous string. In this case one calls setContentLength(length);, send(code, content_type, "");, and then calls sendContent or sendContent_P any number of times.

[prozacgod]

Streaming was the only bit I overlooked, hadn't considered that. Which is another reason I'd be for the refactor, it was non-obvious that this is how streaming an known or unknown length data stream is started.

setContentLength(length);
send(code, "type", ""); 

It's boilerplate one has to grok from sources and leave a comment to remember why you wrote this strange thing. Perhaps purpose driven API's could resolve the strangeness in the current api? And setContentLength could be dropped entirely.

// this can call send header as appropriate, ...UNKNOWN would be appropriate to send here as well.   
startStream(code, type, length);
// or the overloaded variant, just calls startStream with UNKNOWN in length
startStream(code, type);

// then call as seen fit.
sendContent()
...
endStream(); // this can close the client connection

IF it were factored into that (now there'd have issues with compatibility, so discretion is necessary) one could do away with the _contentLength variable. and setContentLength

_contentLength is only branched upon in 2 locations

In one location it's verifying not use _contentLength, and use the size of the content as known in the scope of the current function.

• If set to "NOT_SET" then it simply uses the content length from the data being passed into _prepareHeader which is just to say "bypass this variable in this case anyway"
• if set to CONTENT_LENGTH_UNKNOWN - suppress sending the "Content-Length" header entirely.
• otherwise it will send the _contentLength.

This feels like a lot of weird pomp-and-circumstance to get a stream setup. Especially since send is being used in multiple ways, and it's behavior coaxed to changing by way of state variables. and scope variables.

[igrr]

The startStream/endStream thing looks fine. If you have time, feel free to do a pull request. But I think the old functions have to be kept for a while — there are libraries which happen to use them.

[prozacgod]

Agreed, I expect a lot of beginners and hackers more interested in an end goal using this. Not an audience that wants to update libraries because I changed something... also don't want to get hate mail ;)

Isn't there a standard for gcc/g++ for issuing warnings for deprecated usage? #warning I think, is that something you'd like to include this time around in the setContentLength function, or perhaps say 6 mo from now, and then 6 mo after retire the api (or whatever your preference is, I'll set a reminder and manage a pull request for that timeframe)

[igrr]

__attribute__((deprecated)) lets you do it in gcc.
[[deprecated]] is available in C++14, but we aren't there yet.

[devyte]

@igrr I suspect that the confusion that started this issue is this:

template<typename T> size_t streamFile(T &file, const String& contentType){
  setContentLength(file.size());    //  <--------- setter method called with no purpose
  if (String(file.name()).endsWith(".gz") &&
      contentType != "application/x-gzip" &&
      contentType != "application/octet-stream"){
    sendHeader("Content-Encoding", "gzip");
  }
  send(200, contentType, "");
  return _currentClient.write(file);
}

The setContentLength() method is called, but it seems to me that it serves no purpose calling it in there.

[devyte]

This won't fit into 2.5.0. Pushing milestone back.