[chromium] Plug-in failing to load shouldn't say "Missing Plug-in"
Created attachment 133255 [details] Patch
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.
See also http://crbug.com/119462 for the Chromium bug.
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?
Created attachment 133292 [details] Patch
(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.
Comment on attachment 133292 [details] Patch Clearing flags on attachment: 133292 Committed r111734: <http://trac.webkit.org/changeset/111734>
All reviewed patches have been landed. Closing bug.
I still need to clean up the fallback code.
Created attachment 133779 [details] Patch
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
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
Created attachment 133997 [details] Patch
> 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.
(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.
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?
(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?
> 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.)
(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?
> Do we roll DEPS in a separate patch, or the same? Usually a separate patch.
(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?
(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.
Comment on attachment 133997 [details] Patch Clearing flags on attachment: 133997 Committed r114186: <http://trac.webkit.org/changeset/114186>