Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2284053002:
Register a fake Flash with chrome://plugins during browser start-up. (Closed)
DescriptionRegister a fake Flash with chrome://plugins during browser start-up.
This enables websites to query for the presence of Flash using
navigator.plugins and navigator.mimeTypes even when we don't actually
have it.
TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub
BUG=641448, 641884, 641657
Committed: https://crrev.com/cd59e9b913623e8fec8bbc9d5517610bc1bfc9a7
Cr-Commit-Position: refs/heads/master@{#415100}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Through #5 + git pull && gclient sync #
Total comments: 2
Patch Set 3 : Through #18 #
Total comments: 2
Messages
Total messages: 36 (17 generated)
waffles@chromium.org changed reviewers: + thestig@chromium.org, tommycli@chromium.org, wfh@chromium.org
PTAL. Let me know if you think a better approach is called for; this seemed to be the minimally invasive one.
waffles@chromium.org changed reviewers: + bauerb@chromium.org - tommycli@chromium.org
(Ah, I just realized Tommy is a only per-filed for plugin_power_saver. Sorry for the noise.)
I'm not sure this will compile on a non-chrome build... https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_info_message_filter.cc:10: #include <utility> <algorithm> for std::removeif https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_info_message_filter.cc:409: base::FilePath::FromUTF8Unsafe(ChromeContentClient::kNotPresent); kNotPresent definition is only available for GOOGLE_CHROME_BUILD https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:9: #include <map> unneeded? https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.h:9: #include <set> not needed?
(currently compiling non-Google-build... so slow) https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_info_message_filter.cc:10: #include <utility> On 2016/08/26 22:52:35, Will Harris wrote: > <algorithm> for std::removeif Done. https://codereview.chromium.org/2284053002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_info_message_filter.cc:409: base::FilePath::FromUTF8Unsafe(ChromeContentClient::kNotPresent); On 2016/08/26 22:52:35, Will Harris wrote: > kNotPresent definition is only available for GOOGLE_CHROME_BUILD Whoops! Done... although maybe it's better to just define it for Chromium as well? Let me know. https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:9: #include <map> On 2016/08/26 22:52:35, Will Harris wrote: > unneeded? Same deal, used on 501, decided to include it while I was here. Happy to revert this change. https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/2284053002/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.h:9: #include <set> On 2016/08/26 22:52:35, Will Harris wrote: > not needed? Lint complained at me, so I decided to add it while I was here. Referenced around line 94.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448 ========== to ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 ==========
I wonder if this breaks sites that detect if Flash is enabled and behave differently... what behavior is expected if the user has disabled Flash manually on chrome://plugins - is Flash in navigator.plugins then?
On 2016/08/29 19:55:02, Will Harris wrote: > I wonder if this breaks sites that detect if Flash is enabled and behave > differently... what behavior is expected if the user has disabled Flash manually > on chrome://plugins - is Flash in navigator.plugins then? If you disable Flash manually on chrome://plugins, it will not show in navigator.plugins or navigator.mimeTypes. This is still the behavior after this change: if you go to chrome://plugins and disable Flash (whether the Flash there is real or fake), it will not show in either of those APIs. FWIW, the original bug for this was motivated by a case where a site detected whether or not Flash was available and behaved differently, and we decided we didn't want that.
lgtm
waffles@chromium.org changed reviewers: + sky@chromium.org, thakis@chromium.org - bauerb@chromium.org
+thakis, sky as alternate chrome/ OWNERS. (Would be good to land this soon, as it will also fix a failing official browser test.)
lgtm Erm, you could have just pinged me. https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/plugin_info_message_filter.cc:410: base::FilePath notPresent = nit: not_present
On 2016/08/30 00:17:57, Lei Zhang wrote: > lgtm > > Erm, you could have just pinged me. > > https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... > File chrome/browser/plugins/plugin_info_message_filter.cc (right): > > https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... > chrome/browser/plugins/plugin_info_message_filter.cc:410: base::FilePath > notPresent = > nit: not_present Oh! Sorry, for some reason I thought you were OOO.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
waffles@chromium.org changed reviewers: - sky@chromium.org, thakis@chromium.org
https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2284053002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/plugin_info_message_filter.cc:410: base::FilePath notPresent = On 2016/08/30 00:17:57, Lei Zhang wrote: > nit: not_present Done.
The CQ bit was unchecked by waffles@chromium.org
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2284053002/#ps40001 (title: "Through #18")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sky@chromium.org changed reviewers: + sky@chromium.org
I'm not familiar with this code (and it looks like a hack). Is there another owner that is? https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/plugin_info_message_filter.cc:415: [&](const WebPluginInfo& info) { return info.path == not_present; }), Style guide says don't use default capture. https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.h:31: static const char kNotPresent[]; Please add a description of this.
Message was sent while issue was closed.
Description was changed from ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 ========== to ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 ========== to ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 Committed: https://crrev.com/cd59e9b913623e8fec8bbc9d5517610bc1bfc9a7 Cr-Commit-Position: refs/heads/master@{#415100} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cd59e9b913623e8fec8bbc9d5517610bc1bfc9a7 Cr-Commit-Position: refs/heads/master@{#415100}
Message was sent while issue was closed.
On 2016/08/30 03:25:23, sky wrote: > I'm not familiar with this code (and it looks like a hack). Is there another > owner that is? > > https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... > File chrome/browser/plugins/plugin_info_message_filter.cc (right): > > https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... > chrome/browser/plugins/plugin_info_message_filter.cc:415: [&](const > WebPluginInfo& info) { return info.path == not_present; }), > Style guide says don't use default capture. > > https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... > File chrome/common/chrome_content_client.h (right): > > https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... > chrome/common/chrome_content_client.h:31: static const char kNotPresent[]; > Please add a description of this. Thanks; I will open another CL to address these comments. Berhard would be my preferred OWNER, but is OOO. I agree that this is something of a hack, but it seemed the least-hacky of the alternatives I could think of: certainly it's better than wiring something up in Blink to special-case the presence of Flash in navigator.plugins and navigator.mimetypes. The core issue is that we don't actually have Flash during this small window of time, but since we can fetch it when it is requested, we want the browser to pretend that it does. Using internal-not-yet-present as the path seemed to be following the pattern that the PDF plugin already uses (internal-pdf-viewer); other ideas are of course welcome.
Message was sent while issue was closed.
Isn't there someone else that is an owner of this code that is around? On Wed, Aug 31, 2016 at 12:25 PM, <waffles@chromium.org> wrote: > On 2016/08/30 03:25:23, sky wrote: >> I'm not familiar with this code (and it looks like a hack). Is there >> another >> owner that is? >> >> > https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... >> File chrome/browser/plugins/plugin_info_message_filter.cc (right): >> >> > https://codereview.chromium.org/2284053002/diff/40001/chrome/browser/plugins/... >> chrome/browser/plugins/plugin_info_message_filter.cc:415: [&](const >> WebPluginInfo& info) { return info.path == not_present; }), >> Style guide says don't use default capture. >> >> > https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... >> File chrome/common/chrome_content_client.h (right): >> >> > https://codereview.chromium.org/2284053002/diff/40001/chrome/common/chrome_co... >> chrome/common/chrome_content_client.h:31: static const char kNotPresent[]; >> Please add a description of this. > > Thanks; I will open another CL to address these comments. Berhard would be > my > preferred OWNER, but is OOO. > I agree that this is something of a hack, but it seemed the least-hacky of > the > alternatives I could think of: certainly it's better than wiring something > up in > Blink to special-case the presence of Flash in navigator.plugins and > navigator.mimetypes. The core issue is that we don't actually have Flash > during > this small window of time, but since we can fetch it when it is requested, > we > want the browser to pretend that it does. Using internal-not-yet-present as > the > path seemed to be following the pattern that the PDF plugin already uses > (internal-pdf-viewer); other ideas are of course welcome. > > https://codereview.chromium.org/2284053002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
FWIW bauerb would be the person I would suggest too.
Message was sent while issue was closed.
Description was changed from ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 Committed: https://crrev.com/cd59e9b913623e8fec8bbc9d5517610bc1bfc9a7 Cr-Commit-Position: refs/heads/master@{#415100} ========== to ========== Register a fake Flash with chrome://plugins during browser start-up. This enables websites to query for the presence of Flash using navigator.plugins and navigator.mimeTypes even when we don't actually have it. TEST=https://docs.google.com/a/google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/pub BUG=641448,641884,641657 Committed: https://crrev.com/cd59e9b913623e8fec8bbc9d5517610bc1bfc9a7 Cr-Commit-Position: refs/heads/master@{#415100} ==========
Message was sent while issue was closed.
On 2016/08/31 20:41:13, sky wrote: > Isn't there someone else that is an owner of this code that is around? +tommycli@ is not technically an OWNER but could provide a second opinion. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
