346851 - LOAD_FLAGS_NEW_WINDOW naming doesn't make much sense

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For starters, "what's a window"?  It's not a concept that's really attached well to nsIWebNavigation, at least as the interface stands.

Wouldn't it make more sense to name the flag something like LOAD_FLAGS_FIRST_LOAD and document that this is the first "real" load being performed in this webnavigation (whatever that means)?  That prevents misunderstandings like "well, it's the first load in the tab, but not the first one for the whole window".

The comments should also mention what happens if this flag is set on a non-first load, in my opinion...

Similar naming issue for the docshell internal load flag.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Requesting blocking, since once we ship this in Gecko 1.8.1 we won't be able to change it anymore...

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Decide ASAP, please :)

Comment 3

[Marria Nazif]

Attachment: change variable name and update comment

Comment 4

[Marria Nazif]

Attachment: change variable name and update comment

Comment 5

[Marria Nazif]

Comment on attachment 232338 [details] [diff] [review]
change variable name and update comment

whoops this posted twice for some reason

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 232339 [details] [diff] [review]
change variable name and update comment

this makes one kind of wonder why the caller needs to keep track of whether this is the first load. but this naming does seem better.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 232339 [details] [diff] [review]
change variable name and update comment

> why the caller needs to keep track of whether this is the first
> load

Hence my comments about "real" in comment 0.  We want to exclude the default about:blank load, for example.

>+  // This flag marks the first load in this object
>+  const long INTERNAL_LOAD_FLAGS_FIRST_LOAD              = 0x8;

Add @see nsIWebNavigation::LOAD_FLAGS_FIRST_LOAD.

I really wish I could figure out how to improve the nsIWebNavigation docs here, but nothing is coming to mind.  :(

sr=bzbarsky.  Please land on the 1.8 branch too.  Thanks!

Comment 8

[Marria Nazif]

Comment on attachment 232339 [details] [diff] [review]
change variable name and update comment

checked in on trunk

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 232339 [details] [diff] [review]
change variable name and update comment

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 10

[Brian Ryner (not reading)]

checked in on branch.