Closed Bug 244125 Opened 16 years ago Closed 15 years ago

Polish Plugin Experience - puzzle piece update

Categories

(Toolkit Graveyard :: Plugin Finder Service, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7.4

People

(Reporter: bugs, Assigned: jst)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5)

Attachments

(1 file, 1 obsolete file)

Make sure that all the right keys are set for pre-installed plugins such as 

WMP (ActiveX)
Flash
QT
Real
Assignee: firefox → bugs
Flags: blocking1.0+
Priority: -- → P1
Target Milestone: --- → Firefox1.0
Group: Marketing Private
Status: NEW → ASSIGNED
Target Milestone: Firefox1.0 → Firefox1.0beta
OK, there are several components to this. This is a meta bug. 

This is largely a Windows problem - because linux systems generally have a
Mozilla plugins dir already, and linux users are generally used to installing
this kind of thing. MacOS uses a shared plugin location that gecko detects
correctly and most mac plugins (for Safari, etc) are NPAPI and so the user
doesn't generally have to worry about this. 

For Windows we need to:

1) identify the list of plugins we want to support (see the initial comment in
this bug)

INSTALLER:
2) get the firefox stub installer code landed (from blake)
3) make the stub installer check which plugins are installed (it's a native
windows app - can check reg keys etc) 
4) provide an additional page to the wizard for both "Easy" and "Custom"
installs that lists plugins that were not found on disk (*) to download and
install. 
5) collect xpis that silently install plugins (i.e. without UI) for use in the
installer

CLIENT:
6) create a web plugin service (php or cgi or something) that takes a content
type (e.g. application/x-goat) and translates it into a URL to a location on
stage/ftp where the silent installer for a plugin XPI lives
7) change the client's default pluginspage pref to point to this service,
instead of the netscape one. The behavior should be - user goes to page that
requires plugin, clicks on puzzle piece, is taken to a page that redirects to a
XPI that installs a plugin WITHOUT an interstitial "portal" type page. 
8) Make sure WMP/ActiveX works. (there's a separate bug on this)

(* - maybe also versioning - config.it can say that we ship with version X, if
user has X-1 installed we can install a newer version) 


- We probably need these tasks broken out into individual bugs. Sarah Liberman
will probably take QA on this. 
Assignee: bugs → dveditz
Status: ASSIGNED → NEW
Flags: blocking-aviary1.0RC1+
Assignee: dveditz → jst
lets make this bug for jst's client side puzzle piece replacement.  opening a
new bug for installer side work.  ->
http://bugzilla.mozilla.org/show_bug.cgi?id=253744
Summary: Polish Plugin Experience → Polish Plugin Experience - puzzle piece update
That what I've got right now, works in the cases I've been able to test so far.
What it does is to add a pref and code that makes firefox not load the default
plugin (which we don't need to ship any more, but in case it's picked up from
some other location, this is needed), and makes Gecko fire a "PluginNotFound"
DOM event when an embed/object/applet tag is found for which we don't have a
plugin. This also adds code to display a puzzle piece image and the text "Click
here to download plugin" (which still could be localized, but isn't right now,
so we could deal either way IMO). This isn't trunk material (we've got grander
plans for the trunk, eventually), but should do for the aviary branch, modulo
remaining bugs I haven't found so far.
The frontend part of this is recorded in doron's bug 253046.
Reassigning to doron, he was kind enough to agree to help out here while I'm on
vacation (8/1 to 8/16)
Assignee: jst → doronr
doron, is the patch ready for reviews?   I think we can round up dveditz and ben
and others when it is ready. 
The patch can be reviewed, I tested it today and didn't run into any problems.

The UI this patch adds is an alert(), probably should change it to open the
pluginspage until the other UI goes in.  But the c++ parts are the ones that
need review.
Group: Marketing Private
Attachment #154802 - Flags: superreview?(brendan)
Attachment #154802 - Flags: review?(dbaron)
if this is to go in, it needs to go in soon for testing purposes...
This will need to go in the 1.7 branch as well as Aviary.
brendan/jst/dbaron can you review?
Why would (In reply to comment #10)
> This will need to go in the 1.7 branch as well as Aviary.

Why would we want this on the 1.7 branch? None of the backend webservices stuff
etc is meant to work with 1.7, this is a Firefox feature and I see little value
in bringing this to SeaMonkey now...
Sorry, I thought the "PluginNotFound" event was for Web developers. As long as
it's only for Firefox chrome, no problem.
Are we going to make a 1.X patch?

People have been asking to remove the null plugin forever...
Whiteboard: [have patch]
Marcia has done a lot of preliminary work on Windows plug-in discovery, so I'll
put her as QA contact here.
QA Contact: marcia
Summary: Polish Plugin Experience - puzzle piece update → reverse Polish Plugin Experience - puzzle piece update
Summary: reverse Polish Plugin Experience - puzzle piece update → Polish Plugin Experience - puzzle piece update
(In reply to comment #14)
> Are we going to make a 1.X patch?
> 
> People have been asking to remove the null plugin forever...

Yeah, people have, but for now the idea is to do this only for Firefox, see
comment #12 etc.
Comment on attachment 154802 [details] [diff] [review]
Don't load the default plugin, display puzzle piece and fire events natively when a missing plugin is encounterd

>Index: modules/plugin/base/public/nsIPluginInstanceOwner.idl

Should generate new UUID (unless there's good reason not to -- i.e.,
known extensions that depend on implementations of this interface inside
mozilla and the knowledge that nobody else implements it).

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+  // Get the notification callbacks from the channel and save it as week ref

I know you just moved this, but I prefer month refs or day refs. :-)

>-    if(peer == nsnull)
>+    if(!peer)

And I know you just cleaned this up, but put a space before the paren.


I'm having trouble understanding why the checks for
mDefaultPluginDisabled are where they are (or, rather, why there isn't a
check around the call to PluginNotAvailable).  What prevents the new
stuff from happening when we do have the default plugin?


At some point we really want to make this happen via XBL.  That would be
much less scary, but it requires major work (both of which we should do
at some point) to either:
 a. allow XBL not bound via CSS to lead to frame construction
 b. have pseudo-classes for :-moz-broken-plugin, :-moz-broken-image,
    :-moz-loading-image, etc., which requires moving a lot of plugin
    stuff into content (which we should probably do)
But for the branch, we probably just have to live with this.

>Index: layout/html/base/src/nsObjectFrame.cpp
>+static void
>+FirePluginNotFoundEvent(nsIContent *aTarget)

Can web pages listen to this?  Do we want them to be able to?

>+    nsCOMPtr<nsIDOMHTMLObjectElement> object(do_QueryInterface(mContent));
>+
>+    if (object) {

How can this code ever get hit?  You already returned early if the tag
is object.

>+void
>+SizeDiv(nsIContent *aDiv, PRInt32 aWidth, PRInt32 aHeight)
>+{
>+  nsCOMPtr<nsIDOMElementCSSInlineStyle> element(do_QueryInterface(aDiv));
>+
>+  if (!element)
>+    return;
>+
>+  nsCOMPtr<nsIDOMCSSStyleDeclaration> style;
>+  element->GetStyle(getter_AddRefs(style));
>+
>+  nsCOMPtr<nsIDOMCSS2Properties> css2props(do_QueryInterface(style));
>+
>+  if (!css2props) {
>+    return;
>+  }
>+
>+  nsAutoString tmp;
>+  tmp.AppendInt(aWidth);
>+  AppendASCIItoUTF16("px", tmp);
>+
>+  css2props->SetWidth(tmp);
>+
>+  tmp.Truncate();
>+  tmp.AppendInt(aHeight);
>+  AppendASCIItoUTF16("px", tmp);
>+
>+  css2props->SetHeight(tmp);
>+}

Is there any chance of replacing this with 'width: 100%; height: 100%'
and tweaking the necessary bits of the HTMLReflowState so those work?
That might require making nsObjectFrame::IsContainingBlock return true
under some condition (i.e., the condition that you're doing the puzzle
piece thing).  Though that might not be enough for height.

> nsObjectFrame::Reflow(nsIPresContext*          aPresContext,

>+  if (IsBroken()) {
>+    if (!child) {
>+      CreateDefaultFrames(aPresContext, aMetrics, aReflowState);

It's worth adding a comment here that if you ever switch to something
that uses the frame constructor this will crash in some situations.


>@@ -1364,20 +1490,24 @@ nsObjectFrame::HandleChild(nsIPresContex
>                            nsReflowStatus&          aStatus,
>                            nsIFrame* child)
> {
>   // Note that the child shares our style context, so we simply want
>   // to reflow the child with pretty much our own reflow state....
>   // XXXbz maybe it should have a different style context?
>
>   nsReflowStatus status;
>
>-  ReflowChild(child, aPresContext, aMetrics, aReflowState, 0, 0, 0, status);
>-  FinishReflowChild(child, aPresContext, &aReflowState, aMetrics, 0, 0, 0);
>+  nsHTMLReflowState state(aPresContext, aReflowState, child,
>+                          nsSize(aReflowState.availableWidth,
>+                                 aReflowState.availableHeight));
>+
>+  ReflowChild(child, aPresContext, aMetrics, state, 0, 0, 0, status);
>+  FinishReflowChild(child, aPresContext, &state, aMetrics, 0, 0, 0);

This seems like it might change the behavior for iframe-like-objects and
image-objects.	Have you tested those cases a bit?

>+nsObjectFrame::CreateDefaultFrames(nsIPresContext *aPresContext,
>+    // XXX we're using the same style context for ourselves and the
>+    // image frame.  If this ever changes, please fix HandleChild() to
>+    // deal.

This doesn't seem true anymore.

>+    rv = imgFrame->Init(aPresContext, img, divFrame, imgStyleContext, PR_FALSE);
>+    if (NS_FAILED(rv))
>+      break;
>+
>+    nsHTMLContainerFrame::CreateViewForFrame(imgFrame, divFrame, PR_FALSE);
>+    divFrame->AppendFrames(aPresContext, *shell, nsnull, imgFrame);
>+
>+    rv = NS_NewTextFrame(shell, &textFrame);
>+    if (NS_FAILED(rv))
>+      break;
>+
>+    rv = textFrame->Init(aPresContext, text, divFrame, textStyleContext,
>+                         PR_FALSE);
>+    if (NS_FAILED(rv))
>+      break;
>+
>+    textFrame->SetInitialChildList(aPresContext, nsnull, nsnull);
>+
>+    divFrame->AppendFrames(aPresContext, *shell, nsnull, textFrame);

You could probably use nsFrameList and AppendFrames the imgFrame and the
textFrame all at once.

>Index: layout/html/base/src/nsObjectFrame.h
>+  PRBool IsBroken()
>+  {
>+    return mIsBrokenPlugin;
>+  }

You might want to comment that this is only for the case where we don't
have the default plugin (or whatever the case is, exactly).

>Index: browser/base/content/browser.js

This is temporary, right?
Attachment #154802 - Flags: review?(dbaron) → review+
Attachment #154802 - Attachment is obsolete: true
Attachment #154802 - Flags: superreview?(brendan)
Attachment #156532 - Flags: superreview?(brendan)
Attachment #156532 - Flags: review+
(In reply to comment #17)
> (From update of attachment 154802 [details] [diff] [review])
> >Index: modules/plugin/base/public/nsIPluginInstanceOwner.idl
> 
> Should generate new UUID (unless there's good reason not to -- i.e.,
> known extensions that depend on implementations of this interface inside
> mozilla and the knowledge that nobody else implements it).

Done. To the best of my knowledge this interface is only used internally, so
updating the IID is safe.

> I'm having trouble understanding why the checks for
> mDefaultPluginDisabled are where they are (or, rather, why there isn't a
> check around the call to PluginNotAvailable).  What prevents the new
> stuff from happening when we do have the default plugin?

After looking at that code again, and doing some more testing, I ended up adding
the check you suggested to prevent this code from firing when the default plugin
*is* available.

> At some point we really want to make this happen via XBL.  That would be
> much less scary, but it requires major work (both of which we should do
> at some point) to either:
>  a. allow XBL not bound via CSS to lead to frame construction
>  b. have pseudo-classes for :-moz-broken-plugin, :-moz-broken-image,
>     :-moz-loading-image, etc., which requires moving a lot of plugin
>     stuff into content (which we should probably do)
> But for the branch, we probably just have to live with this.

I couldn't agree more.

> >Index: layout/html/base/src/nsObjectFrame.cpp
> >+static void
> >+FirePluginNotFoundEvent(nsIContent *aTarget)
> 
> Can web pages listen to this?  Do we want them to be able to?

Yes they can, and they can cancel the event etc too. IMO that's fine.

> >+    nsCOMPtr<nsIDOMHTMLObjectElement> object(do_QueryInterface(mContent));
> >+
> >+    if (object) {
> 
> How can this code ever get hit?  You already returned early if the tag
> is object.

Updated the code the unconditionally returned early for all object tags to only
return early if the type of the object tag was a supported document type.

> Is there any chance of replacing this with 'width: 100%; height: 100%'
> and tweaking the necessary bits of the HTMLReflowState so those work?
> That might require making nsObjectFrame::IsContainingBlock return true
> under some condition (i.e., the condition that you're doing the puzzle
> piece thing).  Though that might not be enough for height.

I tried, but no luck. The plugin frame didn't get the right size when I did that.

> >@@ -1364,20 +1490,24 @@ nsObjectFrame::HandleChild(nsIPresContex
> >                            nsReflowStatus&          aStatus,
> >                            nsIFrame* child)
> > {
> >   // Note that the child shares our style context, so we simply want
> >   // to reflow the child with pretty much our own reflow state....
> >   // XXXbz maybe it should have a different style context?
> >
> >   nsReflowStatus status;
> >
> >-  ReflowChild(child, aPresContext, aMetrics, aReflowState, 0, 0, 0, status);
> >-  FinishReflowChild(child, aPresContext, &aReflowState, aMetrics, 0, 0, 0);
> >+  nsHTMLReflowState state(aPresContext, aReflowState, child,
> >+                          nsSize(aReflowState.availableWidth,
> >+                                 aReflowState.availableHeight));
> >+
> >+  ReflowChild(child, aPresContext, aMetrics, state, 0, 0, 0, status);
> >+  FinishReflowChild(child, aPresContext, &state, aMetrics, 0, 0, 0);
> 
> This seems like it might change the behavior for iframe-like-objects and
> image-objects.	Have you tested those cases a bit?

Yes, I did test those cases and they seem to work correctly afaict. But to be on
the safe side, I changed this code to do what it used to do if (!IsBroken()),
and only do the above if we're dealing with a broken plugin.

> >+nsObjectFrame::CreateDefaultFrames(nsIPresContext *aPresContext,
> >+    // XXX we're using the same style context for ourselves and the
> >+    // image frame.  If this ever changes, please fix HandleChild() to
> >+    // deal.
> 
> This doesn't seem true anymore.

Removed that comment from there. It's still true in the case of image and
document "plugins"

> >Index: browser/base/content/browser.js
> 
> This is temporary, right?

Yeah, for testing purposes only.

The rest I did as suggested.
Assignee: doronr → jst
Comment on attachment 156532 [details] [diff] [review]
Updated diff (-w) that reflects dbarons review comments.

+static void
+FirePluginNotFoundEvent(nsIContent *aTarget)

No static needed here, C++ eh?

+  // XXXjst: Error check the above

Is this XXXjst for later, or never?  Could rv |= some more, or something.  If
for later, how about using the FIXME: bug# convention instead?

Looks good otherwise, a well-done "lesser evil" patch.

/be
Attachment #156532 - Flags: superreview?(brendan) → superreview+
Keywords: fixed-aviary1.0
Blocks: 256711
Blocks: 256658
Component: General → Plugin Finder Service
Adding various other regressions from this bug and followups to it to the dep
list so things won't be forgotten when landing on 1.7 and trunk.
Blocks: 256663, 264679, 264684
No longer depends on: 256663
Keywords: aviary-landing
All landed on the 1.7 branch, but the pref is turned off so it still uses the
old default plugin.
Keywords: fixed1.7.5
Fix landed on the trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking-aviary1.1?
Keywords: aviary-landing
This caused bug 277434 (.type broken on object elements).
Blocks: 277434
Blocks: 296749
-[uuid(18270870-32f1-11d2-a830-0040959a28c9)]
+[uuid(f2b9bcb5-95e8-4e15-a439-74b8b45d7846)]
 interface nsIPluginInstanceOwner : nsISupports

fwiw, looks like this change was never checked in...
Product: Firefox → Toolkit
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.