Don't compile code for desktop zoom or GuestViews on mobile platforms.

Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
Review-Url: https://codereview.chromium.org/2874833002
Cr-Commit-Position: refs/heads/master@{#474093}
Committed: https://chromium.googlesource.com/chromium/src/+/336ea2c3ef2261939ee3be7b92da32896343249c

[Kevin McNee, 2017-05-10 17:47:33]

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

[commit-bot: I haz the power, 2017-05-10 17:48: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, 2017-05-10 18:23:01]

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

[commit-bot: I haz the power, 2017-05-10 18:23:02]

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

[Kevin McNee, 2017-05-10 20:32:02]

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

[commit-bot: I haz the power, 2017-05-10 20:34:46]

Dry run: CQ is trying da patch.

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

[Kevin McNee, 2017-05-10 21:37:05]

Description was changed from

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
==========

to

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
==========

[Kevin McNee, 2017-05-10 21:37:06]

mcnee@chromium.org changed reviewers:
+ agrieve@chromium.org

[Kevin McNee, 2017-05-10 21:48:58]

Hi, agrieve. I have a CL to remove some unnecessary code on mobile. I thought
I'd run it by you before sending it off to owners.

This CL consists entirely of gn changes and code deletion/#ifdefing.

With my local release build, this reduces the size of ChromePublic.apk by 10,824
bytes (50,518,987 -> 50,508,163).

[Kevin McNee, 2017-05-10 21:50:30]

Description was changed from

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
==========

to

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
==========

[Kevin McNee, 2017-05-10 21:50:30]

mcnee@chromium.org changed reviewers:
+ agrieve@chromium.org

[Kevin McNee, 2017-05-10 21:51:59]

Hi, agrieve. I have a CL to remove some unnecessary code on mobile. I thought
I'd run it by you before sending it off to owners.

This CL consists entirely of gn changes and code deletion/#ifdefing.

With my local release build, this reduces the size of ChromePublic.apk by 10,824
bytes (50,518,987 -> 50,508,163).

[Avi (use Gerrit), 2017-05-10 21:57:46]

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

[Avi (use Gerrit), 2017-05-10 21:57:46]

FYI I'd be cool with this.

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:49: 
Is this extensions-only? I thought that this was also used by PDF.

[commit-bot: I haz the power, 2017-05-10 22:26:44]

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

[commit-bot: I haz the power, 2017-05-10 22:26:45]

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

[agrieve, 2017-05-11 00:11:48]

On 2017/05/10 21:57:46, Avi (ping after 24h) wrote:
> FYI I'd be cool with this.
> 
>
https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
> File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
> chrome/browser/ui/tab_contents/core_tab_helper.cc:49: 
> Is this extensions-only? I thought that this was also used by PDF.

I'm not at all familiar with the code in question, but the approach lgtm, and
happy to see effort being put into shrinking android :)

[Kevin McNee, 2017-05-11 15:11:54]

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:49: 
On 2017/05/10 21:57:46, Avi (ping after 24h) wrote:
> Is this extensions-only? I thought that this was also used by PDF.

Yeah, the PDF viewer uses GuestViews (specifically, MimeHandlerView), but it is
also an extension.

[Kevin McNee, 2017-05-12 15:55:31]

ping Avi.
Any other concerns before I send this off to remaining owners?

[Avi (use Gerrit), 2017-05-12 15:56:42]

On 2017/05/12 15:55:31, Kevin McNee wrote:
> ping Avi.
> Any other concerns before I send this off to remaining owners?

I'll stamp LGTM, but you definitely need area experts to make sure your surgical
cuts are where they need to be.

[Kevin McNee, 2017-05-12 16:53:41]

mcnee@chromium.org changed reviewers:
+ alokp@chromium.org, bauerb@chromium.org, msw@chromium.org,
tobiasjs@chromium.org, wjmaclean@chromium.org

[Kevin McNee, 2017-05-12 16:53:41]

wjmaclean: Please review changes in
components/guest_view
components/zoom
and double check removal of HostZoomMap and related code from content/

msw: Please review changes in
chrome/browser/ui

bauerb: Please review changes in
chrome/browser/profiles

tobiasjs: Please review changes in
android_webview

alokp: Please review changes in 
chromecast

This CL consists entirely of gn changes and code deletion/#ifdefing.

[Tobias Sargeant, 2017-05-12 17:05:36]

android_webview/ LGTM

[msw, 2017-05-12 18:06:33]

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:633: "//components/guest_view/browser",
Does this belong here? I have no idea what a "guest view" is, or if its related
to extensions, and the class comments in that directory aren't terribly
enlightening. I see that you answered Avi's similar question, but please ask one
of their owners for review of this change, and maybe they'll be inclined to
improve their component name/docs.

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:487: base::Closure renderer_callback
= base::Bind(
It seems like you can include this in a !defined(OS_ANDROID) block, and merge
the two blocks above and below.

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:152: #else   //
BUILDFLAG(ENABLE_EXTENSIONS)
nit: two spaces before comment?

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:250: #else   //
BUILDFLAG(ENABLE_EXTENSIONS)
ditto nit: two spaces before comment?

[wjmaclean, 2017-05-12 19:25:30]

wjmaclean@chromium.org changed reviewers:
+ oshima@chromium.org

[wjmaclean, 2017-05-12 19:27:50]

lgtm for components/ changes

I'm also l-g-t-m on content/ changes, but I think we should get oshima@ to
confirm that either (i) android doesn't rely on use-zoom-for-dsf, or (ii) that
this won't adversely affect use-zoom-for-dsf.

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:153: #else  // OS_ANDROID
? ... Isn't this !OS_ANDROID ?

[Kevin McNee, 2017-05-12 20:28:47]

Regarding use-zoom-for-dsf, I intentionally left the zoom code in blink alone so
that it could still be used by things like dsf. It's just all the browser side
management that's gone, and dsf does not appear to use any of that. But, yeah,
oshima can double check.

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:153: #else  // OS_ANDROID
On 2017/05/12 19:27:50, wjmaclean wrote:
> ? ... Isn't this !OS_ANDROID ?

Yeah, I wasn't sure which to use for these--the original condition or the
negation. Current usage in chromium is inconsistent. But I can switch these over
to the negation.

[alokp, 2017-05-12 22:41:13]

chromecast/ lgtm

[oshima, 2017-05-13 01:49:03]

thank you for heads up. Yes, this will not impact use-zoom-for-dsf work for
android. lgtm

[Bernhard Bauer, 2017-05-15 09:33:28]

lgtm

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:62: #if !defined(OS_ANDROID)
Can you move this up before OS_POSIX etc. so the blocks are approximately
alphabetical?

[Kevin McNee, 2017-05-15 18:22:18]

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:633: "//components/guest_view/browser",
On 2017/05/12 18:06:32, msw wrote:
> Does this belong here? I have no idea what a "guest view" is, or if its
related
> to extensions, and the class comments in that directory aren't terribly
> enlightening. I see that you answered Avi's similar question, but please ask
one
> of their owners for review of this change, and maybe they'll be inclined to
> improve their component name/docs.

I've talked with wjmaclean and lfg offline about this and I've created a bug to
add an explicit build flag for GuestViews instead of relying on extensions (
crbug.com/722453 ). Other GuestView callers are #ifdef'd behind
ENABLE_EXTENSIONS and sometimes !OS_ANDROID, and nothing outside of extensions
currently uses GuestViews, so for now, using enable_extensions here is
reasonable.

[Kevin McNee, 2017-05-15 20:02:10]

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:62: #if !defined(OS_ANDROID)
On 2017/05/15 09:33:28, Bernhard Bauer wrote:
> Can you move this up before OS_POSIX etc. so the blocks are approximately
> alphabetical?

Done.

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:487: base::Closure renderer_callback
= base::Bind(
On 2017/05/12 18:06:32, msw wrote:
> It seems like you can include this in a !defined(OS_ANDROID) block, and merge
> the two blocks above and below.

Done.

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:152: #else   //
BUILDFLAG(ENABLE_EXTENSIONS)
On 2017/05/12 18:06:32, msw wrote:
> nit: two spaces before comment?

So clang-format insists on using 3 spaces following #else, which I'm assuming
isn't intended as it contradicts the style guide. I've uploaded a version with 2
spaces.

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:153: #else  // OS_ANDROID
On 2017/05/12 20:28:47, Kevin McNee wrote:
> On 2017/05/12 19:27:50, wjmaclean wrote:
> > ? ... Isn't this !OS_ANDROID ?
> 
> Yeah, I wasn't sure which to use for these--the original condition or the
> negation. Current usage in chromium is inconsistent. But I can switch these
over
> to the negation.

Done.

[msw, 2017-05-15 20:43:06]

c/b/ui lgtm

[Kevin McNee, 2017-05-15 21:39:27]

mcnee@chromium.org changed reviewers:
+ afakhry@chromium.org, blundell@chromium.org, phajdan.jr@chromium.org

[Kevin McNee, 2017-05-15 21:44:20]

Adding owners for the last few files. PTAL.

afakhry for chrome/browser/task_manager/web_contents_tags.cc
phajdan.jr for chrome/test/base/
blundell for components/BUILD.gn

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
File chrome/browser/ui/tab_contents/core_tab_helper.cc (right):

https://codereview.chromium.org/2874833002/diff/20001/chrome/browser/ui/tab_c...
chrome/browser/ui/tab_contents/core_tab_helper.cc:152: #else   //
BUILDFLAG(ENABLE_EXTENSIONS)
On 2017/05/15 20:02:10, Kevin McNee wrote:
> On 2017/05/12 18:06:32, msw wrote:
> > nit: two spaces before comment?
> 
> So clang-format insists on using 3 spaces following #else, which I'm assuming
> isn't intended as it contradicts the style guide. I've uploaded a version with
2
> spaces.

FYI: I've filled a bug about this. crbug.com/722563

[afakhry, 2017-05-16 00:10:04]

web_contents_tags.cc lgtm.

[blundell, 2017-05-16 13:29:38]

//components/BUILD.gn lgtm

[Kevin McNee, 2017-05-18 14:47:23]

ping phajdan.jr

[Kevin McNee, 2017-05-23 15:25:22]

ping phajdan.jr

[Paweł Hajdan Jr., 2017-05-23 18:25:51]

chrome/test LGTM

[Kevin McNee, 2017-05-23 18:57:18]

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

[commit-bot: I haz the power, 2017-05-23 18:57: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, 2017-05-23 19:04:37]

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

[commit-bot: I haz the power, 2017-05-23 19:04:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin McNee, 2017-05-23 20:54:14]

The CQ bit was checked by mcnee@chromium.org

[Kevin McNee, 2017-05-23 20:54:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avi@chromium.org, oshima@chromium.org, wjmaclean@chromium.org,
agrieve@chromium.org, tobiasjs@chromium.org, alokp@chromium.org,
msw@chromium.org, phajdan.jr@chromium.org, afakhry@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2874833002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-05-23 20:55:08]

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, 2017-05-23 22:51:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495572854296470,
"parent_rev": "b30202c6735437347120a2336f6a43a88888f458", "commit_rev":
"e487064b000cd023327ee014e65143f7450c6dc6"}

[commit-bot: I haz the power, 2017-05-23 22:51:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495572854296470,
"parent_rev": "c051edbbf2aeeb61144a5756752c677ca1147b78", "commit_rev":
"336ea2c3ef2261939ee3be7b92da32896343249c"}

[commit-bot: I haz the power, 2017-05-23 22:51:35]

Description was changed from

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658
==========

to

==========
Don't compile code for desktop zoom or GuestViews on mobile platforms.

Since desktop zoom is not used on mobile, we can remove the host zoom
settings, zoom level management, and related code from mobile builds.

GuestViews are also not needed on mobile, so we remove them as well.

BUG=719658

Review-Url: https://codereview.chromium.org/2874833002
Cr-Commit-Position: refs/heads/master@{#474093}
Committed:
https://chromium.googlesource.com/chromium/src/+/336ea2c3ef2261939ee3be7b92da...
==========

[commit-bot: I haz the power, 2017-05-23 22:51:37]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/336ea2c3ef2261939ee3be7b92da...