RESOLVED FIXED 81907
[chromium] Plug-in failing to load shouldn't say "Missing Plug-in"
https://bugs.webkit.org/show_bug.cgi?id=81907
Summary [chromium] Plug-in failing to load shouldn't say "Missing Plug-in"
Bernhard Bauer
Reported 2012-03-22 07:24:58 PDT
[chromium] Plug-in failing to load shouldn't say "Missing Plug-in"
Attachments
Patch (2.00 KB, patch)
2012-03-22 07:29 PDT, Bernhard Bauer
no flags
Patch (4.17 KB, patch)
2012-03-22 10:32 PDT, Bernhard Bauer
no flags
Patch (1.35 KB, patch)
2012-03-26 05:12 PDT, Bernhard Bauer
no flags
Archive of layout-test-results from ec2-cr-linux-03 (9.71 MB, application/zip)
2012-03-26 05:45 PDT, WebKit Review Bot
no flags
Patch (1.36 KB, patch)
2012-03-27 01:13 PDT, Bernhard Bauer
no flags
Bernhard Bauer
Comment 1 2012-03-22 07:29:56 PDT
WebKit Review Bot
Comment 2 2012-03-22 07:32:36 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Bernhard Bauer
Comment 3 2012-03-22 07:34:29 PDT
See also http://crbug.com/119462 for the Chromium bug.
Adam Barth
Comment 4 2012-03-22 10:16:20 PDT
Comment on attachment 133255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133255&action=review > Source/WebKit/chromium/public/platform/WebLocalizedString.h:78 > + MissingPluginText, Should we sort this list? > Source/WebKit/chromium/src/LocalizedStrings.cpp:213 > + String text = query(WebLocalizedString::MissingPluginText); > + if (!text.isEmpty()) > + return text; > notImplemented(); > return String("Missing Plug-in"); Do you plan to remove this fallback once Chromium is updated? Should we remove the notImplemented() call now that it's implemented?
Bernhard Bauer
Comment 5 2012-03-22 10:32:02 PDT
Bernhard Bauer
Comment 6 2012-03-22 10:33:46 PDT
(In reply to comment #4) > (From update of attachment 133255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133255&action=review > > > Source/WebKit/chromium/public/platform/WebLocalizedString.h:78 > > + MissingPluginText, > > Should we sort this list? Good idea. Done. > > Source/WebKit/chromium/src/LocalizedStrings.cpp:213 > > + String text = query(WebLocalizedString::MissingPluginText); > > + if (!text.isEmpty()) > > + return text; > > notImplemented(); > > return String("Missing Plug-in"); > > Do you plan to remove this fallback once Chromium is updated? Should we remove the notImplemented() call now that it's implemented? Yes, my plan is to remove the fallback and the notImplemented() when we have the strings in Chromium.
WebKit Review Bot
Comment 7 2012-03-22 11:23:30 PDT
Comment on attachment 133292 [details] Patch Clearing flags on attachment: 133292 Committed r111734: <http://trac.webkit.org/changeset/111734>
WebKit Review Bot
Comment 8 2012-03-22 11:23:35 PDT
All reviewed patches have been landed. Closing bug.
Bernhard Bauer
Comment 9 2012-03-26 05:09:02 PDT
I still need to clean up the fallback code.
Bernhard Bauer
Comment 10 2012-03-26 05:12:36 PDT
WebKit Review Bot
Comment 11 2012-03-26 05:45:24 PDT
Comment on attachment 133779 [details] Patch Attachment 133779 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12129822 New failing tests: plugins/embed-attributes-style.html
WebKit Review Bot
Comment 12 2012-03-26 05:45:32 PDT
Created attachment 133784 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Bernhard Bauer
Comment 13 2012-03-27 01:13:49 PDT
Adam Barth
Comment 14 2012-03-27 01:16:46 PDT
> I still need to clean up the fallback code. In the future, please create a new bug for a new patch. Feel free to post a link to the new bug in the old bug.
Bernhard Bauer
Comment 15 2012-03-27 01:17:52 PDT
(In reply to comment #14) > > I still need to clean up the fallback code. > > In the future, please create a new bug for a new patch. Feel free to post a link to the new bug in the old bug. OK, sorry.
Adam Barth
Comment 16 2012-03-27 01:22:57 PDT
Comment on attachment 133997 [details] Patch Certainly not a big deal. :) Do we need to update http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS before landing this patch?
Bernhard Bauer
Comment 17 2012-03-27 01:52:13 PDT
(In reply to comment #16) > (From update of attachment 133997 [details]) > Certainly not a big deal. :) > > Do we need to update http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS before landing this patch? I'm not sure, actually. The string changes landed in http://crrev.com/128891, which is after the revision in DEPS (http://crrev.com/128426), but the test expectations already have the new strings (http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-mac-snowleopard/compositing/plugins/composited-plugin-expected.png, for example). Or is WebKit DEPS only used for building, not for layout tests?
Adam Barth
Comment 18 2012-03-27 01:55:47 PDT
> Or is WebKit DEPS only used for building, not for layout tests? It's used in webkit.org-based builds, but we use chromium.org-based builds to run the LayoutTests. It sounds like we need to update the DEPS file. (It's usually easy, just make sure everything compiles with the larger number.)
Bernhard Bauer
Comment 19 2012-03-27 02:52:41 PDT
(In reply to comment #18) > > Or is WebKit DEPS only used for building, not for layout tests? > > It's used in webkit.org-based builds, but we use chromium.org-based builds to run the LayoutTests. It sounds like we need to update the DEPS file. (It's usually easy, just make sure everything compiles with the larger number.) I updated the DEPS file locally, and ran update-webkit --chromium and build-webkit --chromium, which succeeded. Do we roll DEPS in a separate patch, or the same?
Adam Barth
Comment 20 2012-03-27 13:47:57 PDT
> Do we roll DEPS in a separate patch, or the same? Usually a separate patch.
Bernhard Bauer
Comment 21 2012-03-29 01:33:53 PDT
(In reply to comment #20) > > Do we roll DEPS in a separate patch, or the same? > > Usually a separate patch. OK, it seems http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS has been rolled past http://crrev.com/128891 (after I forgot about it yesterday :-D ) Are we good to land the cleanup now?
Bernhard Bauer
Comment 22 2012-04-13 06:52:11 PDT
(In reply to comment #21) > (In reply to comment #20) > > > Do we roll DEPS in a separate patch, or the same? > > > > Usually a separate patch. > > OK, it seems http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS has been rolled past http://crrev.com/128891 (after I forgot about it yesterday :-D ) > > Are we good to land the cleanup now? Adam, ping? I don't think I can commit the patch myself.
WebKit Review Bot
Comment 23 2012-04-13 17:27:54 PDT
Comment on attachment 133997 [details] Patch Clearing flags on attachment: 133997 Committed r114186: <http://trac.webkit.org/changeset/114186>
WebKit Review Bot
Comment 24 2012-04-13 17:28:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.