superreview granted: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 226606] store state in channel, works f

Darin Fisher (Google) <darin@meer.net> has granted Marria Nazif
<marria@gmail.com>'s request for superreview:
Bug 241972: new window opened by window.open('xxx.exe') or target="_blank"
isn't closed automatically when a download begins
https://bugzilla.mozilla.org/show_bug.cgi?id=241972

Attachment 226606: store state in channel, works for target= as well as
window.open
https://bugzilla.mozilla.org/attachment.cgi?id=226606&action=edit

------- Additional Comments from Darin Fisher (Google) <darin@meer.net>
>Index: docshell/base/nsDocShell.cpp

>+	  props->SetPropertyAsBool(NS_LITERAL_STRING("new-window-target"),

I think it would be a good idea to preface this property name
with the string "docshell." to avoid colliding with other names.
We do the same for the "docshell.internalReferrer" property.


>Index: docshell/base/nsIDocShell.idl

>+  // This flag marks the first load in a new window.	Currently it is
>+  // used in the external helper app service to determine whether it is
>+  // safe to close a window.
>+  const long INTERNAL_LOAD_FLAGS_NEW_WINDOW		   = 0x8;

While it is useful to know where this flag is used, this kind of comment
can easily get out of sync with reality.  Since LXR can tell us where
the flag is used, it might be good to limit this comment to discussing
the functionality of the flag alone.


>Index: docshell/base/nsIWebNavigation.idl

>   /**
>+   * This flag specifies that this load is the first load in a new window.
>+   * Currently it is used to determine whether it is safe to close the
>+   * window in the external helper app service, after downloading the
>+   * content.
>+   */
>+  const unsigned long LOAD_FLAGS_NEW_WINDOW = 0x4000;

Ditto.


>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
...
>+	windowIsNew ? nsIWebNavigation::LOAD_FLAGS_NEW_WINDOW :
>+		      nsIWebNavigation::LOAD_FLAGS_NONE, PR_TRUE);

Here's an example of the flag being used outside of exthandler ;-)


>Index: uriloader/exthandler/Makefile.in

>		  windowwatcher \
>		  embed_base \
>+		    dom \
>		  $(NULL)

Indentation nit.  This Makefile appears to use tabs.  In the wonderful
world of Mozilla, Makefile.in files tend to use tabs whereas everything
else uses spaces in place of tabs :-/


>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

> NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request,
nsISupports *aCtxt, 
>						    nsresult aStatus)
> {
>+  nsCOMPtr<nsIPropertyBag2> props = do_QueryInterface(mRequest);
>+  NS_ENSURE_STATE(props);
>+  props->GetPropertyAsBool(NS_LITERAL_STRING("new-window-target"),
>+			     &mShouldCloseWindow);

Hmm... not every nsIRequest that comes through here will support
nsIPropertyBag2.  I don't think you want to error out if the QI
fails.


>Index: uriloader/exthandler/nsExternalHelperAppService.h

>   /**
>+   * This flag is set if a refresh header was found.	In this case, we
>+   * don't want to close the dom window after handling the content.
>+   */
>+  PRPackedBool mHasRefreshHeader;
>+
>+  /**
>+   * This is set based on whether the channel indicates that a new window
>+   * was opened specifically for this download.  If so, then we
>+   * close it.
>+   */
>+  PRBool mShouldCloseWindow;

s/PRBool/PRPackedBool/


>Index: webshell/public/nsIRefreshURI.idl

>+/**
>+ * Hack Alert: We really want setupRefreshURI to return true if it found
>+ * a refresh header on the channel.  However, we can't modify these
>+ * interfaces on the branch.	So, instead we create a new success code
>+ * that is used to indicate that a refresh header was found and successfully
>+ * setup.
>+ */
>+%{C++
>+#define NS_REFRESHURI_HEADER_FOUND \
>+     NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_URILOADER, \
>+				 2)

This change is going into the trunk as well as the branch, right?  If so,
then I think the trunk comment should not refer to an unnamed branch.  Maybe
it would be best to just make this be the API even though it is not as pretty
as a PRBool return value :-/


sr=darin w/ suggested revisions
0
bugzilla
6/23/2006 1:05:37 AM
mozilla.dev.super-review 29307 articles. 3 followers. Post Follow

0 Replies
969 Views

Similar Articles

[PageSpeed] 41

Reply:

Similar Artilces:

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 226606] store state in channel, works
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 226606: store state in channel, works for target= as well as window.open https://bugzilla.mozilla.org/attachment.cgi?id=226606&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> This is a second iteration on the patch that stores s...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 226289] pass state in the channel
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 226289: pass state in the channel https://bugzilla.mozilla.org/attachment.cgi?id=226289&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> This is another idea I had - we can pass state on the channel which indicates whether a new ...

superreview cancelled: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 226289] pass state in the channel
Marria Nazif <marria@gmail.com> has cancelled Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 226289: pass state in the channel https://bugzilla.mozilla.org/attachment.cgi?id=226289&action=edit ...

superreview granted: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 227564] fixes in response to Darin's re
Darin Fisher (Google) <darin@meer.net> has granted Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 227564: fixes in response to Darin's review https://bugzilla.mozilla.org/attachment.cgi?id=227564&action=edit ------- Additional Comments from Darin Fisher (Google) <darin@meer.net> >Index: docshell/base/nsDocShell.cpp > if (!r...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225294] close blank window
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225294: close blank window https://bugzilla.mozilla.org/attachment.cgi?id=225294&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> This is a first try at closing the window after the external app handler has done its work. This se...

superreview cancelled: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225294] close blank window
Marria Nazif <marria@gmail.com> has cancelled Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225294: close blank window https://bugzilla.mozilla.org/attachment.cgi?id=225294&action=edit ...

superreview cancelled: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 227973] tweaks in response to reviews
Marria Nazif <marria@gmail.com> has cancelled Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 227973: tweaks in response to reviews https://bugzilla.mozilla.org/attachment.cgi?id=227973&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> Actually, I'm going to go ahead and check this in on the trunk, since thes...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 227973] tweaks in response to reviews
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 227973: tweaks in response to reviews https://bugzilla.mozilla.org/attachment.cgi?id=227973&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> (In reply to comment #60) > (From update of attachment 227564 [edit]) > So this w...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 227564] fixes in response to Darin's
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 227564: fixes in response to Darin's review https://bugzilla.mozilla.org/attachment.cgi?id=227564&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> > >+ /** > >+ * This is set based on whether the channel indica...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225651] check that session history is
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225651: check that session history is empty https://bugzilla.mozilla.org/attachment.cgi?id=225651&action=edit ...

superreview cancelled: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225903] docshell tracks whether it lo
Marria Nazif <marria@gmail.com> has cancelled Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225903: docshell tracks whether it loaded any content before https://bugzilla.mozilla.org/attachment.cgi?id=225903&action=edit ...

superreview requested: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225903] docshell tracks whether it lo
Marria Nazif <marria@gmail.com> has asked Darin Fisher (Google) <darin@meer.net> for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225903: docshell tracks whether it loaded any content before https://bugzilla.mozilla.org/attachment.cgi?id=225903&action=edit ------- Additional Comments from Marria Nazif <marria@gmail.com> I added something to the DocShell which tracks if it ever loaded a...

superreview denied: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 225651] check that session history is em
Darin Fisher (Google) <darin@meer.net> has denied Marria Nazif <marria@gmail.com>'s request for superreview: Bug 241972: new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins https://bugzilla.mozilla.org/show_bug.cgi?id=241972 Attachment 225651: check that session history is empty https://bugzilla.mozilla.org/attachment.cgi?id=225651&action=edit ------- Additional Comments from Darin Fisher (Google) <darin@meer.net> Marria and I discussed this today. She's got a much bett...

Bug 241972 new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins
Bug 241972 new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins This bug is filed against Firefox but aren't all these files under core? <https://bugzilla.mozilla.org/attachment.cgi?id=227973&action=view> DocShell, WebNavigation, WindowWatcher, etc. Will this patch be picked up by SeaMonkey trunk? Phil -- Philip Chee <philip@aleytys.pc.my>, <philip.chee@gmail.com> http://flashblock.mozdev.org/ http://xsidebar.mozdev.org Guard us from the she-wolf and the wolf, and guard ...

Web resources about - superreview granted: [Bug 241972] new window opened by window.open('xxx.exe') or target="_blank" isn't closed automatically when a download begins : [Attachment 226606] store state in channel, works f - mozilla.dev.super-review

Resources last updated: 12/18/2015 10:26:07 AM