Remove unused fields from //content.

Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/959fb256dce4f0b1dffa1df094cdff0fc3c7040c
Cr-Commit-Position: refs/heads/master@{#387990}

[Łukasz Anforowicz, 2016-04-07 21:24:32]

Description was changed from

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
==========

to

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-04-14 02:40:56]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2016-04-14 02:40:58]

Nick, could you please take a look?  (I am picking on you because you are a
//content OWNER + because you asked about Scythe on the team's hangout :-)

The first patchset in this CL was generated using Scythe (go/scythe, Googlers
only... :-/ ) to remove unused fields under //content.  Subsequent patchsets
either A) resurrected a field that turned out to be used after all [1] or B)
extended the cleanup (i.e. after removing a field, a constructor would no longer
need a parameter, and callers of the constructor would no longer need to pass an
argument, and so forth).

I hope that this CL can be reviewed in a mechanical way (i.e. just confirming
that I am not removing any actual code / actual usage of the removed fields). 
OTOH, this CL touches all nooks and crannies under //content, so please shout if
you think this CL should be broken down into smaller pieces (I guess regardless
of the decision here, I might want to pull in reviewers for some of the
subdirectories).  For example - removal of |group_id| under appcache/ directory
turned out to cascade rather broadly.

Scythe uses the same dataset that feeds Chromium's code search and has the same
limitation (i.e. knows only about Linux build).  Right now using Scythe requires
considerable manual work to A) figure out which commit Scythe's data applies to
and B) cleanup the CL afterwards to identify code that needs to be retained.  I
hope to eventually address (B) by adding support for an exceptions file(s) and
by storing in such file(s) the fields/code that I've manually discovered have to
be retained (b/28074652).  There is an internal discussion about (A).  FWIW,
I've already landed 2 small CLs (https://crrev.com/1858703003 and
https://crrev.com/1857043002) created using Scythe.

WDYT?

[1] Fields that had to be manually retained:
  - C-style inheritance:
    - _BrowserAccessibilityAtk::parent
    - _BrowserAccessibilityAtkClass::parent_class
  - Platform-specific:
    - ExternalPopupMenu::render_frame_
    - ExternalPopupMenu::popup_menu_client_
    - ExternalPopupMenu::origin_scale_for_emulation_
    - TextInputClientMessageFilter::child_process_id_
    - ShellJavaScriptDialog::manager_
    - AccessibilityEventRecorder::manager_
    - CrosArmGpuProcessPolicy::allow_shmat_
    - MockKeyboard::keyboard_layout_
    - MockKeyboard::keyboard_modifiers_
    - AudioInputRendererHost::AudioEntry::has_keyboard_mic
  - Other:
    - RenderWidgetHostImpl::in_get_backing_store_ (b/28073834)

[ncarter (slow), 2016-04-14 17:37:36]

This seems correct.

There are one or two places where I worry we might just be removing a
recently-added field that is about to be used in a pending CL.

There are only two subparts of this that I think ought to be carved off into
separate CLs: the appcache stuff (because it's big enough) and the leveldb stuff
(because it's under pretty active development, and might need updates to the
class comment).

Everything else seems self-evident enough to land as a big CL.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_internals_ui.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_internals_ui.h:82:
base::WeakPtr<AppCacheServiceImpl> appcache_service_;
The appcache changes are extensive enough that I think it would be a good idea
to move them all to a separate CL. I think it's basically just all the files
under appcache/ and service_worker_disk_cache.cc.

Get a review from michaeln@.

I want to make sure that the fact that the code doesn't reference group_id isn't
a bug that ought to be fixed (Being unfamiliar with this code, I don't know
whether group_id is some kind of security principal that needs to be validated
for access rights, or what)

https://codereview.chromium.org/1875463002/diff/270001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (left):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:371: int
frame_routing_id,
My understanding is that the bluetooth code is in the process of migrating to be
RFH-based. So I don't know that removing this is okay -- we may just be creating
work.

Could you loop ortuno@ in for an opinion on this file?

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
File
content/browser/compositor/browser_compositor_overlay_candidate_validator_mac.h
(right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
content/browser/compositor/browser_compositor_overlay_candidate_validator_mac.h:19:
explicit BrowserCompositorOverlayCandidateValidatorMac();
'explicit' is no longer needed.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
File
content/browser/compositor/browser_compositor_overlay_candidate_validator_ozone.h
(right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
content/browser/compositor/browser_compositor_overlay_candidate_validator_ozone.h:23:
BrowserCompositorOverlayCandidateValidatorOzone(
"explicit" is needed

https://codereview.chromium.org/1875463002/diff/270001/content/browser/leveld...
File content/browser/leveldb_wrapper_impl.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/leveld...
content/browser/leveldb_wrapper_impl.h:16: // This is a wrapper around a
leveldb::LevelDBDatabase. It adds a couple of
This class comment seems inaccurate if we remove |database_|. Or rather, it was
already inaccurate, but now it's not even technically correct.

The member you're removing is only a couple weeks old, and this class is under
active development, so we might just be stomping on somebody's incremental work
here (I'm not sure).
https://chromium.googlesource.com/chromium/src/+/c112794307cc4858616f5aeaa05d...
.

I think you should peel the leveldb wrapper impl .cc/.h changes off into their
own CL, and get erg to review it.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.h:319: 
Remove this extra newline.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:585: gfx::Size
last_swapped_software_frame_size_;
This is also a hapax. It can be deleted.

https://codereview.chromium.org/1875463002/diff/270001/content/common/host_sh...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/1875463002/diff/270001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:25: BitmapData(size_t buffer_size)
: buffer_size(buffer_size) {}
Needs 'explicit'

https://codereview.chromium.org/1875463002/diff/270001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.h (left):

https://codereview.chromium.org/1875463002/diff/270001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.h:821: 
Removal of this line seems like a mistake.

[ortuno, 2016-04-14 17:53:51]

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

[ortuno, 2016-04-14 17:53:53]

https://codereview.chromium.org/1875463002/diff/270001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (left):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:371: int
frame_routing_id,
On 2016/04/14 at 17:37:36, ncarter wrote:
> My understanding is that the bluetooth code is in the process of migrating to
be RFH-based. So I don't know that removing this is okay -- we may just be
creating work.
> 
> Could you loop ortuno@ in for an opinion on this file?

This is OK. We used to save the routing id so we could get the origin but now
that we save the origin in the session we don't need the routing it.

[Łukasz Anforowicz, 2016-04-14 19:34:48]

nick@, can you take another look?

https://codereview.chromium.org/1875463002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_internals_ui.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_internals_ui.h:82:
base::WeakPtr<AppCacheServiceImpl> appcache_service_;
On 2016/04/14 17:37:36, ncarter wrote:
> The appcache changes are extensive enough that I think it would be a good idea
> to move them all to a separate CL. I think it's basically just all the files
> under appcache/ and service_worker_disk_cache.cc.
> 
> Get a review from michaeln@.
> 
> I want to make sure that the fact that the code doesn't reference group_id
isn't
> a bug that ought to be fixed (Being unfamiliar with this code, I don't know
> whether group_id is some kind of security principal that needs to be validated
> for access rights, or what)

Done - https://codereview.chromium.org/1887153002/

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
File
content/browser/compositor/browser_compositor_overlay_candidate_validator_mac.h
(right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
content/browser/compositor/browser_compositor_overlay_candidate_validator_mac.h:19:
explicit BrowserCompositorOverlayCandidateValidatorMac();
On 2016/04/14 17:37:36, ncarter wrote:
> 'explicit' is no longer needed.

Done.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
File
content/browser/compositor/browser_compositor_overlay_candidate_validator_ozone.h
(right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/compos...
content/browser/compositor/browser_compositor_overlay_candidate_validator_ozone.h:23:
BrowserCompositorOverlayCandidateValidatorOzone(
On 2016/04/14 17:37:36, ncarter wrote:
> "explicit" is needed

Done.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/leveld...
File content/browser/leveldb_wrapper_impl.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/leveld...
content/browser/leveldb_wrapper_impl.h:16: // This is a wrapper around a
leveldb::LevelDBDatabase. It adds a couple of
On 2016/04/14 17:37:36, ncarter wrote:
> This class comment seems inaccurate if we remove |database_|. Or rather, it
was
> already inaccurate, but now it's not even technically correct.
> 
> The member you're removing is only a couple weeks old, and this class is under
> active development, so we might just be stomping on somebody's incremental
work
> here (I'm not sure).
>
https://chromium.googlesource.com/chromium/src/+/c112794307cc4858616f5aeaa05d...
> .
> 
> I think you should peel the leveldb wrapper impl .cc/.h changes off into their
> own CL, and get erg to review it.

Done - https://codereview.chromium.org/1886813004/

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.h:319: 
On 2016/04/14 17:37:36, ncarter wrote:
> Remove this extra newline.

Done.

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1875463002/diff/270001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:585: gfx::Size
last_swapped_software_frame_size_;
On 2016/04/14 17:37:36, ncarter wrote:
> This is also a hapax. It can be deleted.

Done.

https://codereview.chromium.org/1875463002/diff/270001/content/common/host_sh...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/1875463002/diff/270001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:25: BitmapData(size_t buffer_size)
: buffer_size(buffer_size) {}
On 2016/04/14 17:37:36, ncarter wrote:
> Needs 'explicit'

Done.

https://codereview.chromium.org/1875463002/diff/270001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.h (left):

https://codereview.chromium.org/1875463002/diff/270001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.h:821: 
On 2016/04/14 17:37:36, ncarter wrote:
> Removal of this line seems like a mistake.

Ooops, I am not sure how I managed to miss this in self-review... :-/
Done.

[ncarter (slow), 2016-04-14 19:43:38]

lgtm

[Łukasz Anforowicz, 2016-04-14 19:56:00]

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

[commit-bot: I haz the power, 2016-04-14 19:56:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1875463002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1875463002/310001

[commit-bot: I haz the power, 2016-04-14 21:52:42]

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

[commit-bot: I haz the power, 2016-04-14 21:52:43]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-04-14 22:03:50]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-04-14 22:05:07]

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

[commit-bot: I haz the power, 2016-04-14 22:23:30]

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

[commit-bot: I haz the power, 2016-04-14 22:23:31]

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

[Łukasz Anforowicz, 2016-04-14 22:25:28]

lukasza@chromium.org changed reviewers:
+ sievers@chromium.org

[Łukasz Anforowicz, 2016-04-14 22:25:29]

sievers@, could you please do an OWNERS review for //gpu?  (for removing an
unused field from 	gpu/ipc/service/gpu_memory_tracking.h/.cc - I didn't notice
that it has moved from content/common/gpu into gpu/ipc/service when I rebased).

[Łukasz Anforowicz, 2016-04-18 17:09:36]

I couldn't get hold of sievers@ on Friday and in the morning today, so I'll just
go ahead with the CL limited to the unused fields under //contents - //gpu
changes are not that important in this CL and landing the rest of the changes
will enable further iterations on Scythe usage (i.e. figuring out how to mark
retained fields so they are not deleted by Scythe in the future).

[Łukasz Anforowicz, 2016-04-18 17:10:06]

Description was changed from

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-04-18 17:10:06]

lukasza@chromium.org changed reviewers:
- sievers@chromium.org

[Łukasz Anforowicz, 2016-04-18 17:13:03]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-04-18 17:13:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1875463002/#ps350001
(title: "Reverting not really needed changes under //gpu.")

[commit-bot: I haz the power, 2016-04-18 17:13:30]

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

[commit-bot: I haz the power, 2016-04-18 20:00:34]

Description was changed from

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-18 20:00:36]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2016-04-18 20:01:45]

Description was changed from

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove unused fields from //content.

This CL was created using Scythe (Googlers only tool, sorry...).
See go/scythe for more details.

BUG=600434
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/959fb256dce4f0b1dffa1df094cdff0fc3c7040c
Cr-Commit-Position: refs/heads/master@{#387990}
==========

[commit-bot: I haz the power, 2016-04-18 20:01:47]

Patchset 19 (id:??) landed as
https://crrev.com/959fb256dce4f0b1dffa1df094cdff0fc3c7040c
Cr-Commit-Position: refs/heads/master@{#387990}