Devtools: Extensible mime typing, mime typing by file name

Extensible mime typing, mime typing by file name

Migrated from https://codereview.chromium.org/1360943002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261
Committed: https://crrev.com/de465d949baa19843653ac320d1f2a7b608e35cd
Cr-Commit-Position: refs/heads/master@{#360498}

[wes, 2015-10-23 19:04:53]

Description was changed from

==========
Extensible mime typing, mime typing by file name

Migrated from https://codereview.chromium.org/1360943002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261
==========

to

==========
Extensible mime typing, mime typing by file name

Migrated from https://codereview.chromium.org/1360943002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261
==========

[wes, 2015-10-23 19:04:53]

wwigham@gmail.com changed reviewers:
+ paulirish@chromium.org, pfeldman@chromium.org

[pfeldman, 2015-10-26 18:21:18]

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
(right):

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:163:
WebInspector.ResourceType.newMimeType = function(extensionOrName, mime, byName)
Copying my review comments here. It seems like the signature of this method is
broken and it is not yet used. Should we inline it in the call sites and name
them broken instead of putting it here?

[wes, 2015-10-26 21:11:46]

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
(right):

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:163:
WebInspector.ResourceType.newMimeType = function(extensionOrName, mime, byName)
On 2015/10/26 18:21:18, pfeldman wrote:
> Copying my review comments here. It seems like the signature of this method is
> broken and it is not yet used. Should we inline it in the call sites and name
> them broken instead of putting it here?

You're correct about the not yet used part - it will only see use in the cl for
the language extension API. But how is it broken? The signature because of the
third parameter? I could see just in-lining this function in the one place it
gets used later pretty easily.

[pfeldman, 2015-10-26 21:23:21]

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
(right):

https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:163:
WebInspector.ResourceType.newMimeType = function(extensionOrName, mime, byName)
> You're correct about the not yet used part - it will only see use in the cl
for
> the language extension API. But how is it broken?

Well, you have two functions and a third that invokes one of them depending on
the third argument.

> I could see just in-lining this function in the one place it
> gets used later pretty easily.

Yes please.

[wes, 2015-10-26 21:27:05]

On 2015/10/26 21:23:21, pfeldman wrote:
>
https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
> (right):
> 
>
https://codereview.chromium.org/1418293002/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:163:
> WebInspector.ResourceType.newMimeType = function(extensionOrName, mime,
byName)
> > You're correct about the not yet used part - it will only see use in the cl
> for
> > the language extension API. But how is it broken?
> 
> Well, you have two functions and a third that invokes one of them depending on
> the third argument.
> 
> > I could see just in-lining this function in the one place it
> > gets used later pretty easily.
> 
> Yes please.

Done. ;)

[pfeldman, 2015-10-26 21:27:49]

lgtm

[wes, 2015-10-26 22:24:06]

The CQ bit was checked by wwigham@gmail.com

[commit-bot: I haz the power, 2015-10-26 22:26:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418293002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418293002/20001

[commit-bot: I haz the power, 2015-10-26 23:28:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 23:28:17]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alph, 2015-10-26 23:43:04]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2015-10-26 23:43:05]

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
(right):

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:150: var
name = WebInspector.TextUtils.fileName(url);
Can't you use ParsedURL functionality? Just not to have this stuff being
implemented twice.

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/TextUtils.js (right):

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/TextUtils.js:160: var
lastIndexOfDot = text.lastIndexOf(delimiter);
nit: not always a dot.

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/TextUtils.js:162: var
indexOfQuestionMark = extension.indexOf("?");
I'd first cut everything past '?' and then would be looking for the extension.
This way you could parse stings like:

maptile.png?lat=12.34&long=56.78

[wes, 2015-11-02 23:25:22]

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/ResourceType.js
(right):

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/ResourceType.js:150: var
name = WebInspector.TextUtils.fileName(url);
On 2015/10/26 23:43:04, alph wrote:
> Can't you use ParsedURL functionality? Just not to have this stuff being
> implemented twice.

The ParsedURL bits don't really replicate this - it doesn't parse out the
extension from the path part. Also, shouldn't we be using the DOM `URL`
constructor where possible? Or is the `ParsedURL` js constructor preferable?
(Neither parse out extensions from the path part, though)

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/TextUtils.js (right):

https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/TextUtils.js:162: var
indexOfQuestionMark = extension.indexOf("?");
On 2015/10/26 23:43:04, alph wrote:
> I'd first cut everything past '?' and then would be looking for the extension.
> This way you could parse stings like:
> 
> maptile.png?lat=12.34&long=56.78

Honestly, this is just the algorithm from
https://codereview.chromium.org/1418293002/diff/20001/third_party/WebKit/Sour...
that's been moved to here. I'm all for improving it, though. I hope nothing
relies on its old behavior.

[paulirish, 2015-11-07 00:31:23]

On 2015/10/26 at 23:28:17, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

wes, here's the test failure:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

[wes, 2015-11-18 22:21:31]

The CQ bit was checked by wwigham@gmail.com

[wes, 2015-11-18 22:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1418293002/#ps40001
(title: "add url function to mock project")

[commit-bot: I haz the power, 2015-11-18 22:22:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418293002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418293002/40001

[commit-bot: I haz the power, 2015-11-19 00:54:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 00:54:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wes, 2015-11-19 01:30:15]

The CQ bit was checked by wwigham@gmail.com

[commit-bot: I haz the power, 2015-11-19 01:36:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418293002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418293002/40001

[commit-bot: I haz the power, 2015-11-19 02:35:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-19 02:36:07]

Patchset 3 (id:??) landed as
https://crrev.com/de465d949baa19843653ac320d1f2a7b608e35cd
Cr-Commit-Position: refs/heads/master@{#360498}