Mojo: HTML Viewer based on Blink.

Mojo: HTML Viewer based on Blink.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278800

[darin (slow to review), 2014-06-20 05:48:38]

There are a ton of TODOs in this code. I think it makes sense to stop here and
snapshot this.

[jamesr, 2014-06-20 05:58:08]

lgtm

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/DEPS (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/DEPS:5: "+third_party/WebKit",
nit: third_party/WebKit/public/

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/blink_platform_impl.cc:7: #include <math.h>
do we do <cmath> ?

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/blink_platform_impl.h (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/blink_platform_impl.h:26: virtual
blink::WebMimeRegistry* mimeRegistry() OVERRIDE;
please don't annotate with OVERRIDE - you'll make someone trying to patch this
on the blink side sad

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/webmimeregistry_impl.h (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/webmimeregistry_impl.h:22: virtual
blink::WebMimeRegistry::SupportsType supportsImageMIMEType(
no OVERRIDEs please

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/weburlloader_impl.cc (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/weburlloader_impl.cc:22:
result.setURL(GURL(url_response->url));
should this have one of those TypeConverter things? i'm not sure when it's
appropriate to use them and when it isn't

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
File mojo/examples/html_viewer/weburlloader_impl.h (right):

https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
mojo/examples/html_viewer/weburlloader_impl.h:29: blink::WebURLError& error,
blink::WebData& data) OVERRIDE;
ditto

[darin (slow to review), 2014-06-20 06:05:58]

On 2014/06/20 at 05:58:08, jamesr wrote:
> lgtm
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/DEPS (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/DEPS:5: "+third_party/WebKit",
> nit: third_party/WebKit/public/

Done.

> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/blink_platform_impl.cc:7: #include <math.h>
> do we do <cmath> ?

I see us doing both. I'll go with cmath.

> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/blink_platform_impl.h (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/blink_platform_impl.h:26: virtual
blink::WebMimeRegistry* mimeRegistry() OVERRIDE;
> please don't annotate with OVERRIDE - you'll make someone trying to patch this
on the blink side sad

D'oh, yes. Thanks for reminding me!

> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/webmimeregistry_impl.h (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/webmimeregistry_impl.h:22: virtual
blink::WebMimeRegistry::SupportsType supportsImageMIMEType(
> no OVERRIDEs please
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/weburlloader_impl.cc (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/weburlloader_impl.cc:22:
result.setURL(GURL(url_response->url));
> should this have one of those TypeConverter things? i'm not sure when it's
appropriate to use them and when it isn't

Perhaps. We could probably save a copy by doing that. I think we have enough
conversions between mojo::String and GURL that I should just file a bug instead
of trying to resolve that here.


> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> File mojo/examples/html_viewer/weburlloader_impl.h (right):
> 
>
https://codereview.chromium.org/340453002/diff/170001/mojo/examples/html_view...
> mojo/examples/html_viewer/weburlloader_impl.h:29: blink::WebURLError& error,
blink::WebData& data) OVERRIDE;
> ditto

[darin (slow to review), 2014-06-20 06:13:30]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-06-20 06:15:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/340453002/190001

[commit-bot: I haz the power, 2014-06-20 09:56:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 10:06:56]

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

[commit-bot: I haz the power, 2014-06-20 10:06:57]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[abarth-chromium, 2014-06-20 14:44:32]

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
File mojo/examples/html_viewer/DEPS (right):

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
mojo/examples/html_viewer/DEPS:5: "+third_party/WebKit",
third_party/WebKit/public ?

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
File mojo/examples/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
mojo/examples/html_viewer/blink_platform_impl.cc:28: "Chrome/35.0.1916.153
Safari/537.36";
Oh, be bold:

"Blink"

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
File mojo/examples/html_viewer/html_document_view.cc (right):

https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
mojo/examples/html_viewer/html_document_view.cc:53:
settings->setAcceleratedCompositingEnabled(false);
We're on a path to deleting this mode.  Can we break html_viewer if it's the
last client?  If so, can you add a comment to that effect here?

[darin (slow to review), 2014-06-20 14:57:00]

On 2014/06/20 at 14:44:32, abarth wrote:
>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> File mojo/examples/html_viewer/DEPS (right):
> 
>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> mojo/examples/html_viewer/DEPS:5: "+third_party/WebKit",
> third_party/WebKit/public ?

Thanks.

> 
>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> File mojo/examples/html_viewer/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> mojo/examples/html_viewer/blink_platform_impl.cc:28: "Chrome/35.0.1916.153
Safari/537.36";
> Oh, be bold:
> 
> "Blink"

Hmm, not trying to change the web platform with this CL ;-)


>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> File mojo/examples/html_viewer/html_document_view.cc (right):
> 
>
https://codereview.chromium.org/340453002/diff/190001/mojo/examples/html_view...
> mojo/examples/html_viewer/html_document_view.cc:53:
settings->setAcceleratedCompositingEnabled(false);
> We're on a path to deleting this mode.  Can we break html_viewer if it's the
last client?  If so, can you add a comment to that effect here?

Yes, I know. That's fine. This should be on compositor bindings soon.

[darin (slow to review), 2014-06-20 15:15:58]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:18:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/340453002/210001

[commit-bot: I haz the power, 2014-06-20 16:05:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19566)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/23173)

[commit-bot: I haz the power, 2014-06-20 16:09:37]

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

[commit-bot: I haz the power, 2014-06-20 16:09:38]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19585)

[darin (slow to review), 2014-06-20 16:18:18]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:20:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/340453002/230001

[commit-bot: I haz the power, 2014-06-20 19:44:06]

Change committed as 278800