Move blink::PluginData, blink::PluginInfo, blink::MimeTypeInfo to oilpan heap.

Move blink::PluginData, blink::PluginInfo, blink::MimeTypeInfo to oilpan heap.

A future patch will fix the spec compliance of navigator.plugins.

BUG=616662
Review-Url: https://codereview.chromium.org/2901353002
Cr-Commit-Position: refs/heads/master@{#475022}
Committed: https://chromium.googlesource.com/chromium/src/+/417f8212676dd6c0e0542e89cfef82e67b03dedf

[lfg, 2017-05-24 22:23:46]

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

[commit-bot: I haz the power, 2017-05-24 22:25:07]

Dry run: CQ is trying da patch.

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

[lfg, 2017-05-24 22:27:24]

lfg@chromium.org changed reviewers:
+ tkent@chromium.org

[lfg, 2017-05-24 22:27:24]

tkent@, can you review?

The plan to fix issue 616662 is to have DOM{Plugin,MimeType}Array to own a
vector of DOM{Plugin,MimeType} that references the objects in the oilpan heap.

[tkent, 2017-05-25 00:10:32]

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMMimeType.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeType.h:43: const MimeClassInfo*
mime_class_info) {
Make the argument "const MimeClassInfo&" to ensure it can't be nullptr.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeType.h:56: explicit
DOMMimeType(LocalFrame*, const MimeClassInfo*);
Do not add |explicit|.
Make the second argument |const MimeClassInfo&|.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMMimeTypeArray.cpp (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeTypeArray.cpp:58: const
HeapVector<Member<MimeClassInfo>>& mimes = data->Mimes();
The local variable |mimes| isn't necessary.

for (const auto& mime : data->Mimes()) {

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMPlugin.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPlugin.h:39: static DOMPlugin*
Create(LocalFrame* frame, const PluginInfo* plugin_info) {
Make the second argument |const PluginInfo&| to ensure it can't be nullptr.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPlugin.h:56: explicit
DOMPlugin(LocalFrame*, const PluginInfo*);
Do not add |explicit|.
Make the second argument |const PluginInfo&| to ensure it can't be nullptr.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMPluginArray.cpp (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPluginArray.cpp:60: for (const
PluginInfo* plugin : plugins) {
The local variable |plugins| isn't necessary.

for (const auto& plugin : data->Plugins()) {

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:39: explicit
MimeClassInfo(String, String, PluginInfo*);
Remove |explicit|
Make the first and second arguments |const String&|, and add argument names.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:43: const String& type()
const { return type_; }
This class is not web-exposed.  So all member functions should start with a
capital letter.

Type()
Desc()
Extensions()
Plugin()

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:68: explicit
PluginInfo(String, String, String);
Remove |explicit|.
Make the all argument |const String&|, and add argument names.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:76: const String& name()
const { return name_; }
Function names should start with a capital letter.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:100: PluginData() {}
Do we need both of the factory function and the public constructor?

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginListBuilder.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginListBuilder.h:57:
HeapVector<Member<PluginInfo>>* results_;
This should be Member<HeapVector<Member<PluginInfo>>>, and this class should be
STACK_ALLOCATED() instead of DISALLOW_NEW().

[commit-bot: I haz the power, 2017-05-25 00:32:18]

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

[commit-bot: I haz the power, 2017-05-25 00:32:19]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-05-25 01:42:29]

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

[lfg, 2017-05-25 01:42:41]

Please, take another look.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMMimeType.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeType.h:43: const MimeClassInfo*
mime_class_info) {
On 2017/05/25 00:10:31, tkent wrote:
> Make the argument "const MimeClassInfo&" to ensure it can't be nullptr.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeType.h:56: explicit
DOMMimeType(LocalFrame*, const MimeClassInfo*);
On 2017/05/25 00:10:32, tkent wrote:
> Do not add |explicit|.
> Make the second argument |const MimeClassInfo&|.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMMimeTypeArray.cpp (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMMimeTypeArray.cpp:58: const
HeapVector<Member<MimeClassInfo>>& mimes = data->Mimes();
On 2017/05/25 00:10:32, tkent wrote:
> The local variable |mimes| isn't necessary.
> 
> for (const auto& mime : data->Mimes()) {

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMPlugin.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPlugin.h:39: static DOMPlugin*
Create(LocalFrame* frame, const PluginInfo* plugin_info) {
On 2017/05/25 00:10:32, tkent wrote:
> Make the second argument |const PluginInfo&| to ensure it can't be nullptr.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPlugin.h:56: explicit
DOMPlugin(LocalFrame*, const PluginInfo*);
On 2017/05/25 00:10:32, tkent wrote:
> Do not add |explicit|.
> Make the second argument |const PluginInfo&| to ensure it can't be nullptr.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/plugins/DOMPluginArray.cpp (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/plugins/DOMPluginArray.cpp:60: for (const
PluginInfo* plugin : plugins) {
On 2017/05/25 00:10:32, tkent wrote:
> The local variable |plugins| isn't necessary.
> 
> for (const auto& plugin : data->Plugins()) {

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:39: explicit
MimeClassInfo(String, String, PluginInfo*);
On 2017/05/25 00:10:32, tkent wrote:
> Remove |explicit|
> Make the first and second arguments |const String&|, and add argument names.

Done. I agree that the argument names are useful, but I had to use bypass-hooks
to upload:

  third_party/WebKit/Source/platform/plugins/PluginData.h:39:  The parameter
name "plugin" adds no information, so it should be removed. 
[readability/parameter_name] [5]

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:43: const String& type()
const { return type_; }
On 2017/05/25 00:10:32, tkent wrote:
> This class is not web-exposed.  So all member functions should start with a
> capital letter.
> 
> Type()
> Desc()
> Extensions()
> Plugin()

Done. I think we need to update the blink coding style if this is the
recommended style.

The blink coding style is outdated and recommends camelCase
(https://www.chromium.org/blink/coding-style) and the chromium coding style
specifies that simple accessors should be unix_hacker_case()
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...).

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:68: explicit
PluginInfo(String, String, String);
On 2017/05/25 00:10:32, tkent wrote:
> Remove |explicit|.
> Make the all argument |const String&|, and add argument names.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:76: const String& name()
const { return name_; }
On 2017/05/25 00:10:32, tkent wrote:
> Function names should start with a capital letter.

Done.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:100: PluginData() {}
On 2017/05/25 00:10:32, tkent wrote:
> Do we need both of the factory function and the public constructor?

No, moved to private.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginListBuilder.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginListBuilder.h:57:
HeapVector<Member<PluginInfo>>* results_;
On 2017/05/25 00:10:32, tkent wrote:
> This should be Member<HeapVector<Member<PluginInfo>>>, and this class should
be
> STACK_ALLOCATED() instead of DISALLOW_NEW().

Done.

[commit-bot: I haz the power, 2017-05-25 01:43:38]

Dry run: CQ is trying da patch.

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

[lfg, 2017-05-25 01:46:19]

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

[commit-bot: I haz the power, 2017-05-25 01:47:13]

Dry run: CQ is trying da patch.

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

[lfg, 2017-05-25 01:47:39]

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:39: explicit
MimeClassInfo(String, String, PluginInfo*);
On 2017/05/25 01:42:40, lfg wrote:
> On 2017/05/25 00:10:32, tkent wrote:
> > Remove |explicit|
> > Make the first and second arguments |const String&|, and add argument names.
> 
> Done. I agree that the argument names are useful, but I had to use
bypass-hooks
> to upload:
> 
>   third_party/WebKit/Source/platform/plugins/PluginData.h:39:  The parameter
> name "plugin" adds no information, so it should be removed. 
> [readability/parameter_name] [5]

Actually, never mind, I just realized that the coding style allows adding just
some arguments names.

[tkent, 2017-05-25 03:17:30]

lgtm.

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/plugins/PluginData.h:43: const String& type()
const { return type_; }
On 2017/05/25 at 01:42:40, lfg wrote:
> On 2017/05/25 00:10:32, tkent wrote:
> > This class is not web-exposed.  So all member functions should start with a
> > capital letter.
> > 
> > Type()
> > Desc()
> > Extensions()
> > Plugin()
> 
> Done. I think we need to update the blink coding style if this is the
recommended style.
> 
> The blink coding style is outdated and recommends camelCase
(https://www.chromium.org/blink/coding-style) and the chromium coding style
specifies that simple accessors should be unix_hacker_case()
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...).

Yeah, it's outdated.  The latest one is
https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:117:
RefPtr<SecurityOrigin> main_frame_origin_;
This member is removable.

[commit-bot: I haz the power, 2017-05-25 03:42:19]

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

[commit-bot: I haz the power, 2017-05-25 03:42:20]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-25 03:49:50]

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

[haraken, 2017-05-25 03:49:50]

LGTM

[jbroman, 2017-05-25 15:27:24]

a couple drive-by nits (lgtm still)

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right):

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:34: void
PluginData::RefreshBrowserSidePluginCache() {
nit: why did this function move up here, far away from the rest of the
PluginData members?

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:40:
DEFINE_TRACE(MimeClassInfo) {
nit: put MimeClassInfo::trace next to the other MimeClassInfo members, etc.?
It's a little odd to have them separated.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:123: String
PluginData::PluginNameForMimeType(const String& mime_type) const {
This function seems to be dead code.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:46: bool
operator==(const MimeClassInfo& info) {
Is this operator== needed anymore? It seems like you've removed the call site.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:76: const String& Desc()
const { return desc_; }
super-nit: while you're touching this, possible to use the full names
("Description", "Filename")? This abbreviation is a little unnecessary.

[lfg, 2017-05-25 21:32:21]

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

[commit-bot: I haz the power, 2017-05-25 21:33:05]

Dry run: CQ is trying da patch.

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

[lfg, 2017-05-25 21:34:42]

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/plugins/PluginData.cpp (right):

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:34: void
PluginData::RefreshBrowserSidePluginCache() {
On 2017/05/25 15:27:24, jbroman wrote:
> nit: why did this function move up here, far away from the rest of the
> PluginData members?

Done.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:40:
DEFINE_TRACE(MimeClassInfo) {
On 2017/05/25 15:27:24, jbroman wrote:
> nit: put MimeClassInfo::trace next to the other MimeClassInfo members, etc.?
> It's a little odd to have them separated.

Done.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.cpp:123: String
PluginData::PluginNameForMimeType(const String& mime_type) const {
On 2017/05/25 15:27:24, jbroman wrote:
> This function seems to be dead code.

Done.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/plugins/PluginData.h (right):

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:46: bool
operator==(const MimeClassInfo& info) {
On 2017/05/25 15:27:24, jbroman wrote:
> Is this operator== needed anymore? It seems like you've removed the call site.

Removed.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:76: const String& Desc()
const { return desc_; }
On 2017/05/25 15:27:24, jbroman wrote:
> super-nit: while you're touching this, possible to use the full names
> ("Description", "Filename")? This abbreviation is a little unnecessary.

Done.

https://codereview.chromium.org/2901353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/plugins/PluginData.h:117:
RefPtr<SecurityOrigin> main_frame_origin_;
On 2017/05/25 03:17:30, tkent wrote:
> This member is removable.

This is still set in UpdatePluginList, since the plugin list depends on the main
frame's origin.

[commit-bot: I haz the power, 2017-05-26 01:21:24]

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

[commit-bot: I haz the power, 2017-05-26 01:21:26]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-05-26 16:02:52]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-05-26 16:02:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2901353002/#ps60001
(title: "nits, remove dead code")

[commit-bot: I haz the power, 2017-05-26 16:03:23]

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-26 16:09:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495814572302380,
"parent_rev": "2779cf5d3b716f5a0f6ef37cf0bd20ee764d675e", "commit_rev":
"417f8212676dd6c0e0542e89cfef82e67b03dedf"}

[commit-bot: I haz the power, 2017-05-26 16:09:56]

Description was changed from

==========
Move blink::PluginData, blink::PluginInfo, blink::MimeTypeInfo to oilpan heap.

A future patch will fix the spec compliance of navigator.plugins.

BUG=616662
==========

to

==========
Move blink::PluginData, blink::PluginInfo, blink::MimeTypeInfo to oilpan heap.

A future patch will fix the spec compliance of navigator.plugins.

BUG=616662

Review-Url: https://codereview.chromium.org/2901353002
Cr-Commit-Position: refs/heads/master@{#475022}
Committed:
https://chromium.googlesource.com/chromium/src/+/417f8212676dd6c0e0542e89cfef...
==========

[commit-bot: I haz the power, 2017-05-26 16:09:58]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/417f8212676dd6c0e0542e89cfef...