Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ContentType text into flash and revise functions #1460

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Oct 5, 2018

  • fromFileExtension() and fromFullFileName() return String instead of const char*
  • Code moved into new file, WebConstants.cpp
  • Bugfix: File extension matching is now done on entire extension, not just the start; this caused a mis-interpretation of 'json' as 'js'.
  • MIME_TYPE_MAP performs a 1:1 mapping between file extension and MIME type. For HTML files the extension has been changed to .html as this appears to be the generally preferred format. The alternative, .htm, is checked in the code when mapping to MIME type.
  • toString() returns an invalid String so the caller can use if() to check for validity. The function previously returned "", which is a rather strange string to be embedding in an HTML header.

* fromFileExtension() and fromFullFileName() return String instead of const char*
* Code moved into new file, WebConstants.cpp
* Bugfix: File extension matching is now done on entire extension, not just the start; this caused a mis-interpretation of 'json' as 'js'.
* MIME_TYPE_MAP performs a 1:1 mapping between file extension and MIME type. For HTML files the extension has been changed to .html as this appears to be the generally preferred format. The alternative, .htm, is checked in the code when mapping to MIME type.
* toString() returns an invalid String so the caller can use if() to check for validity. The function previously returned "<unknown>", which is a rather strange string to be embedding in an HTML header.
@slaff slaff added this to the 3.6.2 milestone Oct 5, 2018
@slaff
Copy link
Contributor

slaff commented Oct 5, 2018

@mikee47 What is your non-mforce-l32 compiler saying about the free heap improvements?

@mikee47
Copy link
Contributor Author

mikee47 commented Oct 5, 2018

Going by the travis builds, and using the HttpServer_WebSockets sample, free RAM goes from 49239 to 49507 for this PR.

@mikee47
Copy link
Contributor Author

mikee47 commented Oct 5, 2018

For the past few PRs we have free RAM / IRAM as:
Fixes to the new PWM 43792 / 6866
base64 rewrites 43788 / 6850
progmem stuff 44768 / 7934
Fix/String_concat 44768 / 7934
feature/optimise-hashmap 44763 / 7934
feature/uri-escaping 44495 / 7934
feature/http-strings 49239 / 7934
feature/improve-WebConstants 49507 / 7934

The uri-escaping result is weird; goes down then goes back up again afterwards!

Update: I discovered the reason is my dodgy patching of http_parser.c. I managed to remove the PROGMEM_L32 attribute on tokens[] (line 195), but the next PR replaced it with PROGMEM.

@mikee47 mikee47 closed this Oct 5, 2018
@mikee47 mikee47 reopened this Oct 5, 2018
@slaff
Copy link
Contributor

slaff commented Oct 5, 2018

... PROGMEM_L32 attribute on tokens[] (line 195), but the next PR replaced it with PROGMEM.

Yes, I saw that one but did not bother you because the next PR was already suggesting proper fix.

Copy link
Contributor

@slaff slaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming issues.

if(!fileName)
return nullptr;

const char* pExt = strrchr(fileName, '.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rename the variable to extension. Try to use more descriptive names. I guess p is a prefix for "pointer" but try to avoid it.

return mimestr_HTML;

for(unsigned i = 0; i < ARRAY_SIZE(extStrings); ++i) {
if(*extStrings[i] == extension)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive name, please.

@slaff slaff removed the 0 - Backlog label Oct 5, 2018
@slaff slaff removed the 3 - Review label Oct 5, 2018
@slaff slaff merged commit fd47e22 into SmingHub:develop Oct 5, 2018
@mikee47 mikee47 deleted the feature/improve_WebConstants branch October 5, 2018 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants