Alice0775 White Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b2pre) Gecko/20091022 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091022 Minefield/3.7a1pre ID:20091022234402

"View Image Info" command is not carried out if "Page Info" window is already opened.

When I carried out multiple "View Image Info"，The command after the second is not carried out unless "Page Info" window is closed.

Reproducible: Always

Steps to Reproduce:
1.Start Minefield with new profile
2.Open URL
3.Carry out "View Image Info" command 
4.Carry out "View Image Info" command  for another image
Actual Results:  
The second command is not carried out

Expected Results:  
The second command should be carried out.
The Page Info" window should be updated,OR one more window should open.

	Philip Chee
	Comment 1 • 15 years ago

What you need is something like:
http://mxr.mozilla.org/comm-central/source/suite/browser/navigator.js#1726
together with:
http://mxr.mozilla.org/comm-central/source/suite/browser/pageinfo/pageInfo.js#351

	Tanner M. Young [:tmyoung] Assignee
	Comment 2 • 15 years ago

Comm-central has http://hg.mozilla.org/comm-central/rev/86f9ec8efa43, which is a modified version of the mozilla-central http://hg.mozilla.org/mozilla-central/rev/59b26c8b271f, which cannot take arguments like the former revision.  I think we can easily do this if we use the elements of the comm-central implementation, I just hope we wont break any extensions in the process.

I will assign this to me, but we will wait on Bug 524106, which Dao is fixing that changes some of this core.  Once he has his changes in the repository, I can quickly fix this one.

	Tanner M. Young [:tmyoung] Assignee
	Comment 3 • 15 years ago

I have taken the comm-central code and added support for imageElement.

Also, I would have simply used the showTab function to select the mediaTab but that left a blank tab open after the contents are cleared, so I used the selector code from loadPageInfo instead.

	Dão Gottwald [:dao]
	Comment 4 • 15 years ago

Comment on attachment 409559 [details] [diff] [review]
Patch for resetPageInfo to accept arguments

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>     	currentWindow.focus();
>-    	return;
>+    	if (inType == "Browser:page-info")
>+    	  currentWindow.resetPageInfo(extraArgument);
>+      return;

Part of the indentation are tabs, please remove them (also before currentWindow.focus()).

>--- a/browser/base/content/pageinfo/pageInfo.js
>+++ b/browser/base/content/pageinfo/pageInfo.js
>@@ -333,17 +333,17 @@ function loadPageInfo()

>+function resetPageInfo(args)

>+  if (args && args.doc) {
>+    gDocument = args.doc;
>+    gWindow = gDocument.defaultView;
>+  }
>+
>+  if (args && args.imageElement)
>+    gImageElement = args.imageElement;
>+
>+  /* Rebuild the data */
>   loadPageInfo();
>+
>+  var initialTab = (args && args.initialTab) || "generalTab";
>+  var radioGroup = document.getElementById("viewGroup");
>+  initialTab = document.getElementById(initialTab) || document.getElementById("generalTab");
>+  radioGroup.selectedItem = initialTab;
>+  radioGroup.selectedItem.doCommand();
>+  radioGroup.focus();
>+  Components.classes["@mozilla.org/observer-service;1"]
>+            .getService(Components.interfaces.nsIObserverService)
>+            .notifyObservers(window, "page-info-dialog-reset", null);

Can that code be moved to a separate function and shared with onLoadPageInfo?

	Tanner M. Young [:tmyoung] Assignee
	Comment 5 • 15 years ago

I have fixed the tabs issue (I have no idea how those got in there...), and I have created the selectTab function.  However, there is a showtab function already, which uses a different selection method that leaves the media list blank if you use it.  For efficiency's sake we probably should merge the two, but I don't know if that will break anything else, like a number of add-ons if we do that.

	Dão Gottwald [:dao]
	Comment 6 • 15 years ago

It looks like onLoadPageInfo and resetPageInfo could share the code that determines gDocument, gWindow and gImageElement, and the loadPageInfo call.

	Tanner M. Young [:tmyoung] Assignee
	Comment 7 • 15 years ago

This uses the idea of combining those extra lines of code.  I believe I prefer this solution, but the other is workable.

	Dão Gottwald [:dao]
	Comment 8 • 15 years ago

Comment on attachment 409850 [details] [diff] [review]
Alternative Patch v 1.1

>+  loadTab((args && args.initialTab) || "generalTab",args);

There's no need for the first argument, since it's part of the second argument.

>+  /* Load the page info */
>+  loadPageInfo()

missing semicolon...

	Tanner M. Young [:tmyoung] Assignee
	Comment 9 • 15 years ago

I fixed the stupid mistakes that Dao caught.

	Dão Gottwald [:dao]
	Comment 10 • 15 years ago

Please integrate toOpenDialogByTypeAndUrl into BrowserPageInfo. The former doesn't belong into the browser.js global scope.

	Tanner M. Young [:tmyoung] Assignee
	Comment 11 • 15 years ago

Changes made.

	Dão Gottwald [:dao]
	Comment 12 • 15 years ago

Comment on attachment 410279 [details] [diff] [review]
Patch v 1.3

>+  while (windows.hasMoreElements()) {
>+    var currentWindow = windows.getNext();
>+    if (currentWindow.document.documentElement.getAttribute("relatedUrl") == documentURL) {
>+      currentWindow.focus();
>+      currentWindow.resetPageInfo(args);
>+      return;

I think this should return currentWindow.

Note that toOpenDialogByTypeAndUrl existed only for BrowserPageInfo, so it can now be removed.

	Tanner M. Young [:tmyoung] Assignee
	Comment 13 • 15 years ago

Removed the extra function and now it returns the window if it is open.

	Dão Gottwald [:dao]
	Comment 14 • 15 years ago

Comment on attachment 410430 [details] [diff] [review]
Patch v 1.4

>+  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+  var windowManagerInterface = windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
>+  var windows = windowManagerInterface.getEnumerator("Browser:page-info");

Use this instead:

var windows = Cc['@mozilla.org/appshell/window-mediator;1']
                .getService(Ci.nsIWindowMediator)
                .getEnumerator("Browser:page-info");

>+  return window.openDialog("chrome://browser/content/pageinfo/pageInfo.xul", "_blank", "chrome,toolbar,dialog=no,resizable", args);

nit: return openDialog(...), without window

>+  Components.classes["@mozilla.org/observer-service;1"]
>+            .getService(Components.interfaces.nsIObserverService)
>+            .notifyObservers(window, "page-info-dialog-reset", null);

Please don't add this notification.

r=me with that

	Dão Gottwald [:dao]
	Comment 15 • 15 years ago

(In reply to comment #14)
> >+  return window.openDialog("chrome://browser/content/pageinfo/pageInfo.xul", "_blank", "chrome,toolbar,dialog=no,resizable", args);
> 
> nit: return openDialog(...), without window

Oh, and break this up into multiple lines, e.g.:

return openDialog("chrome://browser/content/pageinfo/pageInfo.xul", "",
                  "chrome,toolbar,dialog=no,resizable", args);

	Tanner M. Young [:tmyoung] Assignee
	Comment 16 • 15 years ago

Here it is with those last changes.

	Dão Gottwald [:dao]
	Comment 17 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/44ff9f905317

	[not reading bugmail]
	Comment 18 • 15 years ago

V. Fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091106 Minefield/3.7a1pre

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 19 • 15 years ago

Should this have tests? And can I get a quick compatibility assessment for understanding how this might affect add-ons?

	Tanner M. Young [:tmyoung] Assignee
	Comment 20 • 15 years ago

Verified from Comment 18.

	Dão Gottwald [:dao]
	Comment 21 • 15 years ago

(In reply to comment #19)
> can I get a quick compatibility assessment for
> understanding how this might affect add-ons?

The toOpenDialogByTypeAndUrl function has been removed from browser.js, but add-ons don't use that according to <http://mxr-test.konigsberg.mozilla.org/addons/search?string=toOpenDialogByTypeAndUrl>.

	Tanner M. Young [:tmyoung] Assignee
	Comment 22 • 15 years ago

Also the additional arguments added to resetPageInfo shouldn't affect any addons, since no add-ons use that function either.

http://mxr-test.konigsberg.mozilla.org/addons/search?string=resetPageInfo&tree=addons

	Florian Quèze [:florian]
	Comment 23 • 15 years ago

resetPageInfo was added in bug 388504 to make it possible to reload the content of the Page Info window after setting gWindow and gDocument to different values (typically a subframe).
Setting gWindow and gDocument unconditionally in a function called by resetPageInfo breaks this.

resetPageInfo is used in https://addons.mozilla.org/fr/firefox/addon/7457
I guess it doesn't appear in the result of your query because this addon is still in the sandbox.

	Philip Chee
	Comment 24 • 15 years ago

> I guess it doesn't appear in the result of your query because this addon is
> still in the sandbox.
No that MXR instance is an old snapshot and isn't up to date.

	Philip Chee
	Comment 25 • 15 years ago

> Setting gWindow and gDocument unconditionally in a function called by
> resetPageInfo breaks this.

SeaMonkey depends on this to display our Larry information in much the same way you are now using resetPageInfo to display image properties. Fortunately our implementation isn't affected by your changes.

	Tanner M. Young [:tmyoung] Assignee
	Comment 26 • 15 years ago

(In reply to comment #23)
> Setting gWindow and gDocument unconditionally in a function called by
> resetPageInfo breaks this.

We could move the code that defines gDocument and gWindow back to onLoadPageInfo() so that resetting doesn't change those values and then we don't break lines 88 and 89 of your viewframes.js in that add-on.  Moving that code shouldn't break this bug's functionality.  I'll quickly test a patch that does that.

	Tanner M. Young [:tmyoung] Assignee
	Comment 27 • 15 years ago

This is a fix that moves the function that resets the gDocument and gWindow data back to onLoadPageInfo, and it removes the redundant block of code that resets gImageElement from loadTab.

	Dão Gottwald [:dao]
	Comment 28 • 15 years ago

Comment on attachment 410931 [details] [diff] [review]
Fix for regression

args.doc isn't guaranteed to be the same as gDocument when resetPageInfo is called.

	Tanner M. Young [:tmyoung] Assignee
	Comment 29 • 15 years ago

A better fix entirely thanks to Dao.

	Tanner M. Young [:tmyoung] Assignee
	Comment 30 • 15 years ago

This has all of the changes that Dao requested in #developers.

	Dão Gottwald [:dao]
	Comment 31 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/0d83b7b45431

	Dão Gottwald [:dao]
	Comment 32 • 15 years ago

contains the fix for bug 525190

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 33 • 15 years ago

Sadly, Koeningsburg isn't a great reference as it doesn't index all Add-ons, everywhere. It's a good first approximation, though. I'd like to make sure that we don't break any add-on functionality, even if that means creating a 1.9.2 patch that supports old method interfaces for now.

Any reason to not keep toOpenDialogByTypeAndUrl around?

	Dão Gottwald [:dao]
	Comment 34 • 15 years ago

doesn't hurt to keep it

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 35 • 15 years ago

Comment on attachment 410970 [details] [diff] [review]
combined 1.9.2 patch, leaving toOpenDialogByTypeAndUrl untouched

a192=beltzner, thanks for working through this one, all

	Dão Gottwald [:dao]
	Comment 36 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aacc9573ddbd