Closed
Bug 244125
Opened 20 years ago
Closed 20 years ago
Polish Plugin Experience - puzzle piece update
Categories
(Toolkit Graveyard :: Plugin Finder Service, defect, P1)
Toolkit Graveyard
Plugin Finder Service
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)
58.47 KB,
patch
|
jst
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
Make sure that all the right keys are set for pre-installed plugins such as WMP (ActiveX) Flash QT Real
Reporter | ||
Updated•20 years ago
|
Assignee: firefox → bugs
Flags: blocking1.0+
Priority: -- → P1
Target Milestone: --- → Firefox1.0
Reporter | ||
Updated•20 years ago
|
Group: Marketing Private
Status: NEW → ASSIGNED
Target Milestone: Firefox1.0 → Firefox1.0beta
Reporter | ||
Comment 1•20 years ago
|
||
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
Reporter | ||
Updated•20 years ago
|
Flags: blocking-aviary1.0RC1+
Reporter | ||
Updated•20 years ago
|
Assignee: dveditz → jst
Comment 2•20 years ago
|
||
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
Updated•20 years ago
|
Summary: Polish Plugin Experience → Polish Plugin Experience - puzzle piece update
Assignee | ||
Comment 3•20 years ago
|
||
Assignee | ||
Comment 4•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
The frontend part of this is recorded in doron's bug 253046.
Assignee | ||
Comment 6•20 years ago
|
||
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
Comment 7•20 years ago
|
||
doron, is the patch ready for reviews? I think we can round up dveditz and ben and others when it is ready.
Comment 8•20 years ago
|
||
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.
Reporter | ||
Updated•20 years ago
|
Group: Marketing Private
Reporter | ||
Updated•20 years ago
|
Attachment #154802 -
Flags: superreview?(brendan)
Attachment #154802 -
Flags: review?(dbaron)
Comment 9•20 years ago
|
||
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.
Comment 11•20 years ago
|
||
brendan/jst/dbaron can you review?
Assignee | ||
Comment 12•20 years ago
|
||
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.
Comment 14•20 years ago
|
||
Are we going to make a 1.X patch? People have been asking to remove the null plugin forever...
Updated•20 years ago
|
Whiteboard: [have patch]
Comment 15•20 years ago
|
||
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
Updated•20 years ago
|
Summary: Polish Plugin Experience - puzzle piece update → reverse Polish Plugin Experience - puzzle piece update
Updated•20 years ago
|
Summary: reverse Polish Plugin Experience - puzzle piece update → Polish Plugin Experience - puzzle piece update
Assignee | ||
Comment 16•20 years ago
|
||
(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+
Assignee | ||
Comment 18•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #154802 -
Attachment is obsolete: true
Attachment #154802 -
Flags: superreview?(brendan)
Assignee | ||
Updated•20 years ago
|
Attachment #156532 -
Flags: superreview?(brendan)
Attachment #156532 -
Flags: review+
Assignee | ||
Comment 19•20 years ago
|
||
(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 20•20 years ago
|
||
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+
Assignee | ||
Updated•20 years ago
|
Keywords: fixed-aviary1.0
Updated•20 years ago
|
Component: General → Plugin Finder Service
Comment 21•20 years ago
|
||
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.
Updated•20 years ago
|
Updated•20 years ago
|
Keywords: aviary-landing
Updated•20 years ago
|
Whiteboard: [have patch]
Assignee | ||
Comment 22•20 years ago
|
||
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
Assignee | ||
Comment 23•20 years ago
|
||
Fix landed on the trunk.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Flags: blocking-aviary1.1?
Updated•19 years ago
|
Keywords: aviary-landing
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Comment 25•19 years ago
|
||
-[uuid(18270870-32f1-11d2-a830-0040959a28c9)] +[uuid(f2b9bcb5-95e8-4e15-a439-74b8b45d7846)] interface nsIPluginInstanceOwner : nsISupports fwiw, looks like this change was never checked in...
Updated•16 years ago
|
Product: Firefox → Toolkit
Updated•10 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•