Move WebMIMERegistry impl from //content to blink:platform/network/mime

Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879, 659467
Committed: https://crrev.com/67437352849b595c1c3a20ced14c2be67bcea8e2
Cr-Commit-Position: refs/heads/master@{#430130}

[kinuko, 2016-10-24 14:01:59]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 14:02:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-10-24 14:11:06]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/mime

- It's already mojofied so it's relatively simple move

- Reg: directory: alternatively it can be moved to platform/network/mime
  (Wherever we end up with I plan to move all the mime-related code there in
  a follow-up CL)

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/mime

- It's already mojofied

- Reg: directory: alternatively it can be moved to platform/network/mime
  but mime utility is in its own component under components/ and I feel
  either could probably work (suggestions welcome)

 (Wherever we end up with I plan to move all the mime-related code there in
 a follow-up CL)

BUG=561879
==========

[kinuko, 2016-10-24 15:10:03]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/mime

- It's already mojofied

- Reg: directory: alternatively it can be moved to platform/network/mime
  but mime utility is in its own component under components/ and I feel
  either could probably work (suggestions welcome)

 (Wherever we end up with I plan to move all the mime-related code there in
 a follow-up CL)

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/mime

- Reg: directory: alternatively it can be moved to platform/network/mime
  but mime utility is in its own component under components/ and I feel
  either could probably work (suggestions welcome)

 (Wherever we end up with I plan to move all the mime-related code there in
 a follow-up CL)

BUG=561879
==========

[kinuko, 2016-10-24 15:10:03]

kinuko@chromium.org changed reviewers:
+ esprehn@chromium.org

[kinuko, 2016-10-24 15:10:41]

Elliott, can you take a look?

[commit-bot: I haz the power, 2016-10-24 15:14:15]

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

[commit-bot: I haz the power, 2016-10-24 15:14:15]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[esprehn, 2016-10-25 00:28:12]

I think I would do platform/network/mime, the mime related things seem to be in
the net:: namespace in the rest of chromium too?

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp (right):

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:31: return
std::string(reinterpret_cast<const char*>(str.characters8()),
This is already better than what the old code was doing, since it always
inflated to utf16 before converting back to std::string,

UTF16ToASCII(base::StringPiece16(string))

that created a temporary std::string16 to pass into the StringPiece

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:36: return
std::string(stringPiece16.begin(), stringPiece16.end());
You could do std::string(str.characters16(), str.characters16() + str.length())

too

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:88: if
(asciiMimeType.empty())
we can do this before calling ParseCodecString?

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:95: const
WebString& mimeType) {
These should all take WTF::String instead of WebString

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:105: LOG(ERROR)
<< "**** MOJO MIME CALLED!!!";
Why is this an error? It doesn't seem like it should be given the code to
support it below. Can we remove this?

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:111: if
(!m_mimeRegistry->GetMimeTypeFromExtension(fileExtension, &mimeType))
This is a mojo interface, is this doing a sync IPC? :(

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.h (right):

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.h:20: SupportsType
supportsMIMEType(const WebString& mimeType) override;
We don't need to use WebString or virtuals anymore. In fact this doesn't need to
even be on platform, these methods can be static? Or you can have
WebMimeRegistry::instance() ? Ideally we should get rid of most of the big
Platform interface

[kinuko, 2016-10-26 13:36:19]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 13:36:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 13:38:30]

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

[commit-bot: I haz the power, 2016-10-26 13:38:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2016-10-26 15:15:46]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 15:16:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-10-26 15:47:28]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/mime

- Reg: directory: alternatively it can be moved to platform/network/mime
  but mime utility is in its own component under components/ and I feel
  either could probably work (suggestions welcome)

 (Wherever we end up with I plan to move all the mime-related code there in
 a follow-up CL)

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry into network/mime (for DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

[kinuko, 2016-10-26 15:48:48]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry into network/mime (for DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

[commit-bot: I haz the power, 2016-10-26 16:38:47]

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

[commit-bot: I haz the power, 2016-10-26 16:38:48]

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

[kinuko, 2016-10-26 16:47:10]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 16:47:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 16:59:03]

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

[commit-bot: I haz the power, 2016-10-26 16:59:04]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2016-10-26 17:18:22]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[kinuko, 2016-10-26 17:18:27]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-10-26 17:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 17:28:43]

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

[commit-bot: I haz the power, 2016-10-26 17:28:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[kinuko, 2016-10-26 17:55:26]

Patchset #3 (id:40001) has been deleted

[kinuko, 2016-10-27 03:46:37]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 03:46:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-10-27 04:20:44]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

[kinuko, 2016-10-27 04:34:35]

Thanks, updated. As is commented inline I merged WebMIMERegistry impl into
MIMETypeRegistry.cpp, which made the change a little bigger (and introduced one
gotcha).  PTAL

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp (right):

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:36: return
std::string(stringPiece16.begin(), stringPiece16.end());
On 2016/10/25 00:28:11, esprehn wrote:
> You could do std::string(str.characters16(), str.characters16() +
str.length())
> 
> too

Done.

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:88: if
(asciiMimeType.empty())
On 2016/10/25 00:28:11, esprehn wrote:
> we can do this before calling ParseCodecString?

Done.

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:95: const
WebString& mimeType) {
On 2016/10/25 00:28:11, esprehn wrote:
> These should all take WTF::String instead of WebString

Done.

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:105: LOG(ERROR)
<< "**** MOJO MIME CALLED!!!";
On 2016/10/25 00:28:11, esprehn wrote:
> Why is this an error? It doesn't seem like it should be given the code to
> support it below. Can we remove this?

*facepalm* sure, it was just for local logging

https://codereview.chromium.org/2444873002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mime/WebMimeRegistryImpl.cpp:111: if
(!m_mimeRegistry->GetMimeTypeFromExtension(fileExtension, &mimeType))
On 2016/10/25 00:28:11, esprehn wrote:
> This is a mojo interface, is this doing a sync IPC? :(

Yes, apparently that is what we do in the existing code :(

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/MIMETypeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/MIMETypeRegistry.h:37: // TODO(kinuko): Move
this file under platform/network/mime.
Will make this move once this patch lands (including the change makes this patch
way bigger)

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:104: // the
local mapping.
As your comment it didn't look we needed WebMimeRegistry interface exposed on
Platform any longer so I just removed WebMimeRegistry and merged the impl into
MIMETypeRegistry.cpp.

...then after doing so I realized now we have new tests (added after my initial
upload) that require test-only impl for this, so I added test-only code path
here.  I'd like to hear what you think about this-- I can possibly have
WebMimeRegistry and virtuals back, while having this small fallback code here
doesn't look too bad to me either (compared with all the Platform thing we had
before).

[commit-bot: I haz the power, 2016-10-27 05:14:51]

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

[commit-bot: I haz the power, 2016-10-27 05:14:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[esprehn, 2016-10-27 05:20:25]

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:1: /*
Use the short copyright for all new files, just update this to 2016

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:104: // the
local mapping.
I think you want to install a mock MimeRegistry service that returns whatever
you want in the tests. That's the magic of mojo, you mock out at the IPC layer.

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:119:
WebStringToFilePath(WebString(ext)).value(), &mimeType);
ext should be enough, it should implicitly convert to WebString?

[esprehn, 2016-10-27 05:22:39]

esprehn@chromium.org changed reviewers:
+ dcheng@chromium.org, reillyg@chromium.org

[esprehn, 2016-10-27 05:22:40]

See PasswordInputTypeTest.cpp for an example of mocking at the mojo layer. We
shouldn't need to add unit test specific paths here.

I'm also going on vacation tomorrow until next tuesday, so dcheng@ and reillyg@
might be able to help you get this landed before then. :)

[kinuko, 2016-10-27 16:35:12]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 16:35:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 18:17:41]

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

[commit-bot: I haz the power, 2016-10-27 18:17:42]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-10-27 22:31:24]

Unfortunately Elliot's confidence in my is misplaced as I am not a Blink OWNER.

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:17: class
MockMimeRegistryProvider : public blink::InterfaceProvider {
This seems like an unnecessary level of abstraction. Why not just export the
MockMimeRegistry type from this class?

[kinuko, 2016-10-28 00:32:40]

Thanks Elliott!

On 2016/10/27 22:31:24, Reilly Grant wrote:
> Unfortunately Elliot's confidence in my is misplaced as I am not a Blink
OWNER.

If you can review mojo-related change that's helpful enough :)

[kinuko, 2016-10-28 00:40:07]

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:1: /*
On 2016/10/27 05:20:25, esprehn wrote:
> Use the short copyright for all new files, just update this to 2016

Done.

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:104: // the
local mapping.
On 2016/10/27 05:20:25, esprehn wrote:
> I think you want to install a mock MimeRegistry service that returns whatever
> you want in the tests. That's the magic of mojo, you mock out at the IPC
layer.

Cool, done.

https://codereview.chromium.org/2444873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:119:
WebStringToFilePath(WebString(ext)).value(), &mimeType);
On 2016/10/27 05:20:25, esprehn wrote:
> ext should be enough, it should implicitly convert to WebString?

Done.

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mime/DEPS (right):

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mime/DEPS:1: include_rules = [
This file is a remnant of old cl, will remove

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:17: class
MockMimeRegistryProvider : public blink::InterfaceProvider {
On 2016/10/27 22:31:23, Reilly Grant wrote:
> This seems like an unnecessary level of abstraction. Why not just export the
> MockMimeRegistry type from this class?

I wanted to have something like InterfaceProvider::TestApi via
TestingPlatformSupport's interface provider (so modules can add and remove its
mock impl into it), and thought doing so via each module's mock interface
provider (which anyways seem to be often written by module authors) might be
nice.  I can make this indirection go away too for sure.

[kinuko, 2016-10-28 00:48:40]

Ok, removed the indirection. PTAL

rellyg: mojo-related changes
dcheng: overall

network/mime/MIMETypeRegistry.cpp has the largest change, I think Elliott's done
a basic review for it. Most remaining changes are relatively mechanical

[Reilly Grant (use Gerrit), 2016-10-28 01:04:27]

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:17: class
MockMimeRegistryProvider : public blink::InterfaceProvider {
On 2016/10/28 at 00:40:07, kinuko wrote:
> On 2016/10/27 22:31:23, Reilly Grant wrote:
> > This seems like an unnecessary level of abstraction. Why not just export the
> > MockMimeRegistry type from this class?
> 
> I wanted to have something like InterfaceProvider::TestApi via
TestingPlatformSupport's interface provider (so modules can add and remove its
mock impl into it), and thought doing so via each module's mock interface
provider (which anyways seem to be often written by module authors) might be
nice.  I can make this indirection go away too for sure.

In that case we still want to be registering service factory functions with the
interface provider, not whole other interface provider implementations.

[kinuko, 2016-10-28 01:07:15]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879,659467
==========

[kinuko, 2016-10-28 01:09:15]

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:17: class
MockMimeRegistryProvider : public blink::InterfaceProvider {
On 2016/10/28 01:04:26, Reilly Grant wrote:
> On 2016/10/28 at 00:40:07, kinuko wrote:
> > On 2016/10/27 22:31:23, Reilly Grant wrote:
> > > This seems like an unnecessary level of abstraction. Why not just export
the
> > > MockMimeRegistry type from this class?
> > 
> > I wanted to have something like InterfaceProvider::TestApi via
> TestingPlatformSupport's interface provider (so modules can add and remove its
> mock impl into it), and thought doing so via each module's mock interface
> provider (which anyways seem to be often written by module authors) might be
> nice.  I can make this indirection go away too for sure.
> 
> In that case we still want to be registering service factory functions with
the
> interface provider, not whole other interface provider implementations.

Yeah using interface provider as an indirection is probably not the right way,
that's what I was also afraid when I created the patch (I was trying a few
things)-- anyways this part's now gone.

[Reilly Grant (use Gerrit), 2016-10-28 01:12:41]

https://codereview.chromium.org/2444873002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2444873002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:73:
mojo::MakeRequest<mojom::blink::MimeRegistry>(std::move(handle))));
Instead of saving the MockMimeRegistry here, which only allows a single
connection in the lifetime of the interface provider, I would instead say,
"mojo::MakeStrongBinding(wrapUnique(new MockMimeRegistry()),
mojo::MakeRequest<mojom::blink::MimeRegistry>(std::move(handle)))" and remove
the mojo:Binding field from MockMimeRegistry. This way the mock implementation
has the same lifetime as the pipe connected to it.

[kinuko, 2016-10-28 01:18:22]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[kinuko, 2016-10-28 01:18:46]

https://codereview.chromium.org/2444873002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2444873002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:73:
mojo::MakeRequest<mojom::blink::MimeRegistry>(std::move(handle))));
On 2016/10/28 01:12:41, Reilly Grant wrote:
> Instead of saving the MockMimeRegistry here, which only allows a single
> connection in the lifetime of the interface provider, I would instead say,
> "mojo::MakeStrongBinding(wrapUnique(new MockMimeRegistry()),
> mojo::MakeRequest<mojom::blink::MimeRegistry>(std::move(handle)))" and remove
> the mojo:Binding field from MockMimeRegistry. This way the mock implementation
> has the same lifetime as the pipe connected to it.

Cool, done.

[commit-bot: I haz the power, 2016-10-28 01:19:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Reilly Grant (use Gerrit), 2016-10-28 01:35:44]

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:75: static
mojom::blink::MimeRegistryPtr mimeRegistry = getMimeRegistryPtr();
Sorry I didn't notice this earlier but static variables of a non-primitive type
are not allowed by the Google C++ style guide.

You should either define a singleton that owns the Mojo pipe or there isn't much
of an added cost to just opening a new pipe for each of these calls as long as
you update MimeRegistryImpl to maintain a binding set instead of creating a new
instance for each connection.

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:8: #include
"mojo/public/cpp/bindings/binding.h"
nit: This header is no longer used.

[commit-bot: I haz the power, 2016-10-28 02:39:21]

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

[commit-bot: I haz the power, 2016-10-28 02:39:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[kinuko, 2016-10-28 15:02:11]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 15:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-10-28 15:03:33]

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:75: static
mojom::blink::MimeRegistryPtr mimeRegistry = getMimeRegistryPtr();
On 2016/10/28 01:35:44, Reilly Grant wrote:
> Sorry I didn't notice this earlier but static variables of a non-primitive
type
> are not allowed by the Google C++ style guide.
> 
> You should either define a singleton that owns the Mojo pipe or there isn't
much
> of an added cost to just opening a new pipe for each of these calls as long as
> you update MimeRegistryImpl to maintain a binding set instead of creating a
new
> instance for each connection.

Ok thanks for catching, was under an impression that this is a ptr or POD-like
holder but realized not really.  Created a wrapper holder type for that.

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h (right):

https://codereview.chromium.org/2444873002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MockMimeRegistry.h:8: #include
"mojo/public/cpp/bindings/binding.h"
On 2016/10/28 01:35:44, Reilly Grant wrote:
> nit: This header is no longer used.

Done.

[commit-bot: I haz the power, 2016-10-28 16:38:33]

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

[commit-bot: I haz the power, 2016-10-28 16:38:34]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-28 23:50:30]

I'm not sure if std::string <-> WTF::String conversions have been a problem
elsewhere, but it definitely feels awkward.

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:36:
std::string ToASCIIOrEmpty(const String& str) {
This feels like a slightly surprising API. In //base, the corresponding API is
just UTF16ToASCII, which DCHECKs that the string is ASCII.

(I also wonder if these helpers are something we want to put in a shared header
somewhere? They like general std::string interop utils, which I guess we'll
possibly need elsewhere?)

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:140:
m_interfaceProvider(wrapUnique(new TestingInterfaceProvider)) {
Nit: no wrapUnique needed here

[kinuko, 2016-11-01 14:02:48]

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp
(right):

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp:36:
std::string ToASCIIOrEmpty(const String& str) {
On 2016/10/28 23:50:30, dcheng wrote:
> This feels like a slightly surprising API. In //base, the corresponding API is
> just UTF16ToASCII, which DCHECKs that the string is ASCII.

For a bit more context, this is a slightly efficient version of
SimpleWebMimeRegistryImpl::ToASCIIOrEmpty
(https://cs.chromium.org/chromium/src/content/child/simple_webmimeregistry_imp...),
which actually calls base::UTF16ToASCII() but only if base::IsStringASCII()
returns true in the original code.

> (I also wonder if these helpers are something we want to put in a shared
header
> somewhere? They like general std::string interop utils, which I guess we'll
> possibly need elsewhere?)

I can try looking into other callsites of UTF16ToASCII to see if having a helper
like this in a more generic place helps them too as a follow-up.

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2444873002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:140:
m_interfaceProvider(wrapUnique(new TestingInterfaceProvider)) {
On 2016/10/28 23:50:30, dcheng wrote:
> Nit: no wrapUnique needed here

Done.

[kinuko, 2016-11-01 14:10:22]

On 2016/10/28 23:50:30, dcheng wrote:
> I'm not sure if std::string <-> WTF::String conversions have been a problem
> elsewhere, but it definitely feels awkward.

The original code was doing following in a typical case:
1. WTFString::lower() [copy]
2. base::Latin1OrUTF16ToUTF16() to make it string16 conversion [copy]
3. base::UTF16ToASCII() if base::IsStringASCII() returns true [copy]

So in 8-bit ASCII cases (which must be the majority) we first convert it to
utf16 and then copy it back as ASCII std::string, which feels wasting.  The new
version tries to perform minimal string conversion/copy.

[kinuko, 2016-11-01 14:10:33]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 14:10:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 15:45:42]

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

[commit-bot: I haz the power, 2016-11-01 15:45:44]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-11-03 01:11:20]

On 2016/10/28 23:50:30, dcheng wrote:
> (I also wonder if these helpers are something we want to put in a shared
header
> somewhere? They like general std::string interop utils, which I guess we'll
> possibly need elsewhere?)

Multiple other callsites of UTF16ToASCII (or ASCIIToUTF16) seem to be doing
similar conversions which could be possibly saved, so I gave it a shot in a
separate patch:
https://codereview.chromium.org/2469353003/

[kinuko, 2016-11-04 11:39:29]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 11:39:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 13:10:03]

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

[commit-bot: I haz the power, 2016-11-04 13:10:04]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-11-04 14:53:51]

Rebased. PTAL?

[Reilly Grant (use Gerrit), 2016-11-04 15:11:28]

still lgtm

[esprehn, 2016-11-04 22:47:20]

lgtm

[kinuko, 2016-11-05 02:35:08]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2016-11-05 02:35:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 02:41:01]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879,659467
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879,659467
==========

[commit-bot: I haz the power, 2016-11-05 02:41:03]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-11-05 02:43:37]

Description was changed from

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879,659467
==========

to

==========
Move WebMIMERegistry impl from //content to blink:platform/network/mime

This change does:
- Remove WebMIMERegistry from Platform interface
- Merge WebMIMERegistry impl into blink::MIMETypeRegistry
- Move MIMETypeRegistry.cpp into network/mime (for cleaner DEPS)

I will move MIMETypeRegistry.h and other mime-related files into the
same directory in a follow-up patch.

BUG=561879,659467

Committed: https://crrev.com/67437352849b595c1c3a20ced14c2be67bcea8e2
Cr-Commit-Position: refs/heads/master@{#430130}
==========

[commit-bot: I haz the power, 2016-11-05 02:43:39]

Patchset 10 (id:??) landed as
https://crrev.com/67437352849b595c1c3a20ced14c2be67bcea8e2
Cr-Commit-Position: refs/heads/master@{#430130}