Bug 81907 – [chromium] Plug-in failing to load shouldn't say "Missing Plug-in"

• Component: New Bugs
• Status: RESOLVED FIXED

[Bernhard Bauer]

[chromium] Plug-in failing to load shouldn't say "Missing Plug-in"

[Bernhard Bauer]

Created attachment 133255 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Bernhard Bauer]

See also http://crbug.com/119462 for the Chromium bug.

[Adam Barth]

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]

Created attachment 133292 [details]
Patch

[Bernhard Bauer]

(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 on attachment 133292 [details]
Patch

Clearing flags on attachment: 133292

Committed r111734: <http://trac.webkit.org/changeset/111734>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bernhard Bauer]

I still need to clean up the fallback code.

[Bernhard Bauer]

Created attachment 133779 [details]
Patch

[WebKit Review Bot]

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]

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]

Created attachment 133997 [details]
Patch

[Adam Barth]

> 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]

(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 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]

(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]

> 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]

(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]

> Do we roll DEPS in a separate patch, or the same?

Usually a separate patch.

[Bernhard Bauer]

(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]

(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 on attachment 133997 [details]
Patch

Clearing flags on attachment: 133997

Committed r114186: <http://trac.webkit.org/changeset/114186>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.