Implement Just-In-Time Flash updates.

Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it can handle as a plugin during registration with the component updater. If plugin content is encountered that we do not have a plugin for, the filter will ask the component updater if an implementation is available. If so, it will initiate an on-demand update for the plugin and register an observer to watch for the update completion. In the meantime, the renderer will display a plugin placeholder indicating that a download is in progress. Once/if the update is successful, the placeholder is notified and will re-evaluate the presence of a suitable plugin for the content. This second pass should catch any blocked-by-policy, power-saver, etc restrictions.

BUG=624067
Committed: https://crrev.com/77255cc35690dc6985a460eeea28cb51b3dff26f
Cr-Commit-Position: refs/heads/master@{#409217}

[waffles, 2016-07-26 22:36:00]

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

[commit-bot: I haz the power, 2016-07-26 22:36:30]

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-07-26 22:47:47]

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

[commit-bot: I haz the power, 2016-07-26 22:47:48]

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

[waffles, 2016-07-27 18:44:29]

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

[commit-bot: I haz the power, 2016-07-27 18:44:48]

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-07-27 18:51:30]

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

[commit-bot: I haz the power, 2016-07-27 18:51:31]

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...)

[waffles, 2016-07-27 18:59:52]

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

[commit-bot: I haz the power, 2016-07-27 19:00:18]

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-07-27 19:13:25]

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

[commit-bot: I haz the power, 2016-07-27 19:13:26]

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

[waffles, 2016-07-27 21:18:15]

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

[commit-bot: I haz the power, 2016-07-27 21:18:27]

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-07-27 21:57:04]

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

[commit-bot: I haz the power, 2016-07-27 21:57:05]

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

[waffles, 2016-07-27 22:10:53]

Patchset #1 (id:1) has been deleted

[waffles, 2016-07-27 22:10:56]

Patchset #1 (id:20001) has been deleted

[waffles, 2016-07-27 22:11:07]

Patchset #1 (id:40001) has been deleted

[waffles, 2016-07-27 22:11:25]

Patchset #1 (id:60001) has been deleted

[waffles, 2016-07-27 22:11:30]

Patchset #1 (id:80001) has been deleted

[waffles, 2016-07-27 22:11:43]

Patchset #1 (id:100001) has been deleted

[waffles, 2016-07-27 22:16:46]

Description was changed from

==========
Implement Just-In-Time Flash updates.

The basic approach is that we hardcode a map of MIME types to component
IDs, and then if we have no plugin for those MIME types, display a
placeholder indicating that we are downloading the plugin and start an
on-demand update for it. Once/if the update is successful, we notify the
placeholder that the plugin list has changed, which will result in it
re-evaluating which placeholder (if any) to display. This second pass
should catch any blocked-by-policy, power-saver, etc restrictions.

BUG=624067
==========

to

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass
should catch any blocked-by-policy, power-saver, etc restrictions.

BUG=624067
==========

[waffles, 2016-07-27 22:19:41]

Description was changed from

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass
should catch any blocked-by-policy, power-saver, etc restrictions.

BUG=624067
==========

to

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass should catch any blocked-by-policy,
power-saver, etc restrictions.

BUG=624067
==========

[waffles, 2016-07-27 22:19:53]

waffles@chromium.org changed reviewers:
+ bauerb@chromium.org, sorin@chromium.org

[waffles, 2016-07-27 22:19:54]

Sorin, Bernhard, PTAL.

[Bernhard Bauer, 2016-07-28 10:25:11]

Nice!

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:273:
main_thread_task_runner_->PostTask(
You could do this with PostTaskAndReply().

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:477: plugin_metadata =
base::WrapUnique(new PluginMetadata(
You could even use base::MakeUnique<PluginMetadata>() :)

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:260: using Events =
update_client::UpdateClient::Observer::Events;
Should this come inside of the public: section?

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:268: routing_id_(routing_id),
I think these should be aligned with |observer_|?

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:273: if (id != component_id_) {
Nit: no braces for single-line bodies.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:300: std::string component_id_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:311: #endif
Another advantage of using unique_ptr<> in the map: The destructor would delete
any leftover observers automatically.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.h (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:52: friend class ComponentObserver;
Could you make this an inner class? That way it would automatically have access
to private members, and not linger around in the global namespace.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:75: // Stores all
PluginPlaceholderHosts, keyed by their routing ID.
Nit: stores all ComponentObservers?

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:76: std::map<int, ComponentObserver*>
component_observers_;
This map could store unique_ptr<>'s now (|plugin_placeholders_| dates from
before that worked).

https://codereview.chromium.org/2154773002/diff/120001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/common/render_m...
chrome/common/render_messages.h:468:
IPC_MESSAGE_ROUTED2(ChromeViewHostMsg_BlockedUpdatePlugin,
Nit: the name of the message doesn't make much sense... maybe
_BlockedComponentUpdatedPlugin?

https://codereview.chromium.org/2154773002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:208:
IPC_MESSAGE_HANDLER(ChromeViewMsg_PluginComponentUpdateDownloading,
Couldn't you merge these message handlers into the block above? Just keep the
messages that are only defined with ENABLE_PLUGIN_INSTALLATION inside of the
#ifdef block.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service.cc:203: if (component ==
NULL)
nullptr :) Or just directly use it in the check.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service.cc:206:
base::UTF8ToUTF16(component->name.c_str(), component->name.size(),
I would use the version of UTF8ToUTF16 that takes a single StringPiece and
returns the UTF16 string.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/component_updater_service_internal.h (right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service_internal.h:115: using
ComponentIdsByMimeType = std::map<std::string, std::string>;
Nit: This type is never used anywhere else, and it's not much shorter than the
full type... :)

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/default_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/default_component_installer_unittest.cc:106: 
Nit: remove one empty line.

[waffles, 2016-07-28 20:06:28]

Thanks Bernhard! PTAL.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:273:
main_thread_task_runner_->PostTask(
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> You could do this with PostTaskAndReply().

Hm, I avoided this the first time because (IIUC) it requires me to introduce a
ref-counted struct to hold output and plugin_metadata, and that seemed like it
needlessly complicated the ownership.

I don't mind switching to that approach, but just wanted to check that this was
the intent.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:477: plugin_metadata =
base::WrapUnique(new PluginMetadata(
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> You could even use base::MakeUnique<PluginMetadata>() :)

Done. (used reset().)

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:260: using Events =
update_client::UpdateClient::Observer::Events;
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Should this come inside of the public: section?

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:268: routing_id_(routing_id),
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> I think these should be aligned with |observer_|?

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:273: if (id != component_id_) {
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Nit: no braces for single-line bodies.

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:300: std::string component_id_;
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:311: #endif
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Another advantage of using unique_ptr<> in the map: The destructor would
delete
> any leftover observers automatically.

Acknowledged.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.h (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:52: friend class ComponentObserver;
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Could you make this an inner class? That way it would automatically have
access
> to private members, and not linger around in the global namespace.

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:75: // Stores all
PluginPlaceholderHosts, keyed by their routing ID.
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Nit: stores all ComponentObservers?

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:76: std::map<int, ComponentObserver*>
component_observers_;
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> This map could store unique_ptr<>'s now (|plugin_placeholders_| dates from
> before that worked).

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/common/render_m...
chrome/common/render_messages.h:468:
IPC_MESSAGE_ROUTED2(ChromeViewHostMsg_BlockedUpdatePlugin,
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Nit: the name of the message doesn't make much sense... maybe
> _BlockedComponentUpdatedPlugin?

Done.

https://codereview.chromium.org/2154773002/diff/120001/chrome/renderer/plugin...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/renderer/plugin...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:208:
IPC_MESSAGE_HANDLER(ChromeViewMsg_PluginComponentUpdateDownloading,
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Couldn't you merge these message handlers into the block above? Just keep the
> messages that are only defined with ENABLE_PLUGIN_INSTALLATION inside of the
> #ifdef block. 

Done.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service.cc:203: if (component ==
NULL)
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> nullptr :) Or just directly use it in the check.

Done.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service.cc:206:
base::UTF8ToUTF16(component->name.c_str(), component->name.size(),
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> I would use the version of UTF8ToUTF16 that takes a single StringPiece and
> returns the UTF16 string.

Done.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/component_updater_service_internal.h (right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/component_updater_service_internal.h:115: using
ComponentIdsByMimeType = std::map<std::string, std::string>;
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Nit: This type is never used anywhere else, and it's not much shorter than the
> full type... :)

Done.

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
File components/component_updater/default_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2154773002/diff/120001/components/component_u...
components/component_updater/default_component_installer_unittest.cc:106: 
On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> Nit: remove one empty line.

Done.

[waffles, 2016-07-28 20:50:26]

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

[commit-bot: I haz the power, 2016-07-28 20:50:47]

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-07-28 20:58:44]

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

[commit-bot: I haz the power, 2016-07-28 20:58:45]

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

[Sorin Jianu, 2016-07-28 21:04:06]

Thank you! Small fry below.

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:406: auto observer = new
ComponentObserver(this, placeholder_id, identifier);
Clarify the ownership of the ComponentObserver by using a unique_ptr and
transfer it over to the map before exiting the function.

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:418: NOTREACHED();
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

"not handle DCHECK() failures"

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
File components/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
components/component_updater/component_updater_service.h:114: ComponentInfo*
info) const = 0;
Another idea for this would be to return by value an
std::unique_ptr<ComponentInfo> or nullptr instead of bool and the comment about
not mutating |info|.

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
File components/component_updater/default_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
components/component_updater/default_component_installer_unittest.cc:107: return
std::vector<std::string>()
isn't this missing a ; at the end of line?

[Sorin Jianu, 2016-07-28 21:04:12]

Thank you! Small fry below.

[waffles, 2016-07-28 23:05:39]

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

[waffles, 2016-07-28 23:05:52]

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:406: auto observer = new
ComponentObserver(this, placeholder_id, identifier);
On 2016/07/28 21:04:05, Sorin Jianu wrote:
> Clarify the ownership of the ComponentObserver by using a unique_ptr and
> transfer it over to the map before exiting the function.

Done.

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:418: NOTREACHED();
On 2016/07/28 21:04:05, Sorin Jianu wrote:
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> 
> "not handle DCHECK() failures"

Done.

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
File components/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
components/component_updater/component_updater_service.h:114: ComponentInfo*
info) const = 0;
On 2016/07/28 21:04:05, Sorin Jianu wrote:
> Another idea for this would be to return by value an
> std::unique_ptr<ComponentInfo> or nullptr instead of bool and the comment
about
> not mutating |info|.

Done.

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
File components/component_updater/default_component_installer_unittest.cc
(right):

https://codereview.chromium.org/2154773002/diff/140001/components/component_u...
components/component_updater/default_component_installer_unittest.cc:107: return
std::vector<std::string>()
On 2016/07/28 21:04:06, Sorin Jianu wrote:
> isn't this missing a ; at the end of line?

Done.

[commit-bot: I haz the power, 2016-07-28 23:06:02]

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

[Sorin Jianu, 2016-07-28 23:29:39]

Thank you!

https://codereview.chromium.org/2154773002/diff/160001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/2154773002/diff/160001/components/component_u...
components/component_updater/default_component_installer.h:89: // If this
component is a plugin, returns the media types it can handle as a
nit.

Can erase " as a vector. Else, does nothing"

[Sorin Jianu, 2016-07-28 23:29:48]

lgtm

[waffles, 2016-07-28 23:42:05]

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

[commit-bot: I haz the power, 2016-07-28 23:42:40]

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

[waffles, 2016-07-28 23:42:44]

https://codereview.chromium.org/2154773002/diff/160001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/2154773002/diff/160001/components/component_u...
components/component_updater/default_component_installer.h:89: // If this
component is a plugin, returns the media types it can handle as a
On 2016/07/28 23:29:39, Sorin Jianu wrote:
> nit.
> 
> Can erase " as a vector. Else, does nothing"

Done.

[commit-bot: I haz the power, 2016-07-29 00:32:55]

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

[commit-bot: I haz the power, 2016-07-29 00:32:55]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-07-29 09:14:24]

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:273:
main_thread_task_runner_->PostTask(
On 2016/07/28 20:06:27, waffles wrote:
> On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> > You could do this with PostTaskAndReply().
> 
> Hm, I avoided this the first time because (IIUC) it requires me to introduce a
> ref-counted struct to hold output and plugin_metadata, and that seemed like it
> needlessly complicated the ownership.
> 
> I don't mind switching to that approach, but just wanted to check that this
was
> the intent.

No, the callbacks would be exactly the same. The only difference is you wouldn't
have to pass the second callback or the task runner into the first callback.

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:418: NOTREACHED();
On 2016/07/28 23:05:52, waffles wrote:
> On 2016/07/28 21:04:05, Sorin Jianu wrote:
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > 
> > "not handle DCHECK() failures"
> 
> Done.

I would be careful there -- this is handling an IPC message from an untrusted
process, which could send us anything. I think most STL map implementations
won't do something too bad when dereferencing their end() iterator, but it still
seems somewhat dangerous. So, technically we should probably handle this error
and call bad_message::ReceivedBadMessage() to kill the renderer process.
(Although as you can see in chrome/browser/bad_message.h, we are not very good
at actually following through on this.)

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:304:
cus->RemoveObserver(observer.second.get());
If you move this to the ComponentObserver destructor (maybe together with
AddObserver() in the constructor so it's symmetric), it would automatically
happen both here and in RemoveComponentObserver().

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.h (right):

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:50: class ComponentObserver : public
update_client::UpdateClient::Observer {
I think you could just declare the class here and move the definition to the .cc
file.

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:66: friend class ComponentObserver;
I don't think this friend declaration is now necessary anymore.

[waffles, 2016-07-29 20:57:22]

Patchset #5 (id:200001) has been deleted

[waffles, 2016-07-29 21:07:25]

Thanks again! PTAL.

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:273:
main_thread_task_runner_->PostTask(
On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> On 2016/07/28 20:06:27, waffles wrote:
> > On 2016/07/28 10:25:10, Bernhard Bauer wrote:
> > > You could do this with PostTaskAndReply().
> > 
> > Hm, I avoided this the first time because (IIUC) it requires me to introduce
a
> > ref-counted struct to hold output and plugin_metadata, and that seemed like
it
> > needlessly complicated the ownership.
> > 
> > I don't mind switching to that approach, but just wanted to check that this
> was
> > the intent.
> 
> No, the callbacks would be exactly the same. The only difference is you
wouldn't
> have to pass the second callback or the task runner into the first callback.

I think I'm still confused about what you mean here - in particular, we don't
pass the second callback here at all; but anyways while looking at this I think
I found a nicer way to express it, so... maybe done? PTAL.

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:418: NOTREACHED();
On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> On 2016/07/28 23:05:52, waffles wrote:
> > On 2016/07/28 21:04:05, Sorin Jianu wrote:
> > >
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > > 
> > > "not handle DCHECK() failures"
> > 
> > Done.
> 
> I would be careful there -- this is handling an IPC message from an untrusted
> process, which could send us anything. I think most STL map implementations
> won't do something too bad when dereferencing their end() iterator, but it
still
> seems somewhat dangerous. So, technically we should probably handle this error
> and call bad_message::ReceivedBadMessage() to kill the renderer process.
> (Although as you can see in chrome/browser/bad_message.h, we are not very good
> at actually following through on this.)

In this case, RemoveComponentObserver is only triggered by
PluginObserver::ComponentObserver::OnEvent, and routing_id comes from that
class's member (not the event). Originally this ID is from the renderer, but
it's an invariant that the ID should exist in component_observers_ if we created
a ComponentObserver with that ID; so I think the DCHECK is safe here.

(Note that this is asymmetrical with OnRemovePluginPlaceholderHost, below - I've
reverted the behavior there to the original.)

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:304:
cus->RemoveObserver(observer.second.get());
On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> If you move this to the ComponentObserver destructor (maybe together with
> AddObserver() in the constructor so it's symmetric), it would automatically
> happen both here and in RemoveComponentObserver().

Good idea, thanks. Done.

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.h (right):

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:50: class ComponentObserver : public
update_client::UpdateClient::Observer {
On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> I think you could just declare the class here and move the definition to the
.cc
> file.

Done.

https://codereview.chromium.org/2154773002/diff/180001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.h:66: friend class ComponentObserver;
On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> I don't think this friend declaration is now necessary anymore.

Done.

[Bernhard Bauer, 2016-08-01 09:30:39]

LGTM!

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_observer.cc (right):

https://codereview.chromium.org/2154773002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_observer.cc:418: NOTREACHED();
On 2016/07/29 21:07:25, waffles wrote:
> On 2016/07/29 09:14:24, Bernhard Bauer wrote:
> > On 2016/07/28 23:05:52, waffles wrote:
> > > On 2016/07/28 21:04:05, Sorin Jianu wrote:
> > > >
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> > > > 
> > > > "not handle DCHECK() failures"
> > > 
> > > Done.
> > 
> > I would be careful there -- this is handling an IPC message from an
untrusted
> > process, which could send us anything. I think most STL map implementations
> > won't do something too bad when dereferencing their end() iterator, but it
> still
> > seems somewhat dangerous. So, technically we should probably handle this
error
> > and call bad_message::ReceivedBadMessage() to kill the renderer process.
> > (Although as you can see in chrome/browser/bad_message.h, we are not very
good
> > at actually following through on this.)
> 
> In this case, RemoveComponentObserver is only triggered by
> PluginObserver::ComponentObserver::OnEvent, and routing_id comes from that
> class's member (not the event). Originally this ID is from the renderer, but
> it's an invariant that the ID should exist in component_observers_ if we
created
> a ComponentObserver with that ID; so I think the DCHECK is safe here.
> 
> (Note that this is asymmetrical with OnRemovePluginPlaceholderHost, below -
I've
> reverted the behavior there to the original.)

Oh, right. Cool!

https://codereview.chromium.org/2154773002/diff/220001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/220001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:271:
main_thread_task_runner_.get(), FROM_HERE,
Indent two more spaces.

[waffles, 2016-08-01 18:13:46]

https://codereview.chromium.org/2154773002/diff/220001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2154773002/diff/220001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:271:
main_thread_task_runner_.get(), FROM_HERE,
On 2016/08/01 09:30:39, Bernhard Bauer wrote:
> Indent two more spaces.

Done.

[waffles, 2016-08-01 18:13:53]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-08-01 18:13:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2154773002/#ps240001
(title: "Through #48 + format")

[commit-bot: I haz the power, 2016-08-01 18:14:24]

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-08-01 18:17:58]

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

[commit-bot: I haz the power, 2016-08-01 18:18:00]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[waffles, 2016-08-01 21:25:38]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-08-01 21:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sorin@chromium.org
Link to the patchset: https://codereview.chromium.org/2154773002/#ps260001
(title: "git pull && gclient sync")

[commit-bot: I haz the power, 2016-08-01 21:26:00]

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-08-01 21:33:12]

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

[commit-bot: I haz the power, 2016-08-01 21:33:13]

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

[waffles, 2016-08-01 21:35:43]

waffles@chromium.org changed reviewers:
+ sky@chromium.org

[waffles, 2016-08-01 21:35:44]

+sky@ for chrome/common/render_message.h and
chrome/renderer/chrome_content_renderer_client.cc (although please feel invited
to take a look at the rest, too!)

[sky, 2016-08-01 23:20:38]

chrome/renderer/chrome_content_renderer_client.cc LGTM, You need security for
render_messages.h

[waffles, 2016-08-01 23:28:07]

waffles@chromium.org changed reviewers:
+ jschuh@chromium.org, rsesek@chromium.org

[waffles, 2016-08-01 23:28:09]

+rsesek, PTAL at renderer_messages.h.

[Robert Sesek, 2016-08-02 14:36:48]

_messages LGTM

[waffles, 2016-08-02 17:19:39]

The CQ bit was checked by waffles@chromium.org

[commit-bot: I haz the power, 2016-08-02 17:20:09]

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-08-02 17:25:29]

Description was changed from

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass should catch any blocked-by-policy,
power-saver, etc restrictions.

BUG=624067
==========

to

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass should catch any blocked-by-policy,
power-saver, etc restrictions.

BUG=624067
==========

[commit-bot: I haz the power, 2016-08-02 17:25:31]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-08-02 17:27:06]

Description was changed from

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass should catch any blocked-by-policy,
power-saver, etc restrictions.

BUG=624067
==========

to

==========
Implement Just-In-Time Flash updates.

The basic approach is that every component supplies a list of MIME types that it
can handle as a plugin during registration with the component updater. If plugin
content is encountered that we do not have a plugin for, the filter will ask the
component updater if an implementation is available. If so, it will initiate an
on-demand update for the plugin and register an observer to watch for the update
completion. In the meantime, the renderer will display a plugin placeholder
indicating that a download is in progress. Once/if the update is successful, the
placeholder is notified and will re-evaluate the presence of a suitable plugin
for the content. This second pass should catch any blocked-by-policy,
power-saver, etc restrictions.

BUG=624067

Committed: https://crrev.com/77255cc35690dc6985a460eeea28cb51b3dff26f
Cr-Commit-Position: refs/heads/master@{#409217}
==========

[commit-bot: I haz the power, 2016-08-02 17:27:07]

Patchset 7 (id:??) landed as
https://crrev.com/77255cc35690dc6985a460eeea28cb51b3dff26f
Cr-Commit-Position: refs/heads/master@{#409217}