154559 - comments in nsDocShell.cpp need an editor

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[timeless]

1921     // XXX in that case docshell hierarchyand SH hierarchy won't match.
5936     // The model is really if we don't know differently then add which 
basically
5937     // means we are suppose to try all the things we know not to allow in 
and
5938     // then if we don't bail go on and allow it in.  But here lets compare
5939     // against the most common case we know to allow in and go on and say 
yes
5940     // to it.

problems: 'and' is not a suffix
'we are supposeD to'
that sentence is too long.
'here let<'>s compare'
if we don't bail go on ... <- missing punctuation
most common case we know to allow in ... <- missing punctuation

Comment 1

[timeless]

ok, well i forgot that sleep() thing, so here comes...

note that there are a few places where i inserted my opinion so this patch 
probably can't be checked in as is.

Comment 2

[timeless]

Attachment: draft

Comment 3

[timeless]

Comment on attachment 89400 [details] [diff] [review]
draft

there's an error in
@@ -5290,25 +5345,26 @@

for now pretend the line pair is:
-	 return NS_OK;		 // URIs not the same
+	 return NS_OK;		 // URIs not the same

Comment 4

[Boris Zbarsky [:bzbarsky]]

> +                     * loading a new page in this child. Check parent's and
> self's busy status and if it is, 

If it is _what_?

> +                         * load will automatically happen everytime, no
> matter how the page is loaded.

"every time"

> +        /* We need an owner (a referring principal).
> +         * there are three possibilities:

"There are" (new sentence, capitalize).

> +  /* Does the origin have the same principal or a subdomain of the target's
> document.domain

Question mark, please.

> -                } // else (no parent) allow this load since shell is a
> toplevel window
> +                } // else (no parent) allow this load since shell is a top
> level window

I'd leave this one be.

> +    /* Second we check our children making sure not to ask a child if it

Comma after "Second".

> +    /* Third if we have a parent and it isn't the requestor then we should
> ask

Comma after "Third".

> -         * a new url has been loaded on you. 
> +         * a new url has been loaded on you. ??

Why this change?

> +    /* i don't want to read this pref in every time we load a url

Capitalize the "i".

> +    /* Update SessionHistory with the document's title. Unless
> +     * page was loaded from history or the page bypassed history.

"title, unless"

> +    // Shouldn't this be UNIMEPLEMENTED or something??

UNIMPLEMENTED, and same elsewhere (lose the "E" after the first "M").

Make those "This is unreachable" comments into assertions?

> +// else after return

No need to comment those....

> +     * optional, if it is absent, the currently loaded url is used.

"optional; if it is absent"

> +        // this should be wyciwygChannel, dropping letters only confuses
> things
> +        // alternatively it could have a pretty name like 'generatedChannel'

Don't complain; just change it if you care.

> +  // This looks like a job for NS_ENSURE_FALSE

Same here.

> +                     * So, the best we can do, is to tear down the new window

No comma after "do", please.

> +            /* This is an anchor traversal with in the same page.

"within"

> +     * If this is a HTTP channel, then set up the HTTP specific information

"an HTTP"

> +     * XXX: Is seems wrong that the owner is ignored - even if one is
> +     *      supplied) unless the URI is javascript or data.

'-' and ')' do not match as delimiters...

> +     * Turn nsXPIDLCString into an nsString.
> +     * (need to find the new string APIs so we don't do this

File a bug on me on this, please, with line number or something.

> +     * This means that comparing "http://www.ABC.com/" to
> "http://www.abc.com/"
> +     * will fail this test.
> +     */
> +    // This sounds broken

File another bug on me, please, as above.

> +     * that could just be me... If the intent is only deny about:blank then
> we

"to deny only"

> +     * But first we'll optimize for the common cases, we know to add.

No comma before "we know", please.

> +     * setup to point to the focused, or content window, so we have to

"set up", not "setup".

Comment 5

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.