Closed Bug 1129795 Opened 9 years ago Closed 9 years ago

Convert docshell/ to Gecko style

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(6 files, 1 obsolete file)

smaug, based on the IRC discussion[0] yesterday, I presume you would accept patches to reformat docshell/ to match modern Gecko style? The patches would be similar to those in bug 1046841.

Do you want to review the patches or should I ask someone else? froydnj was a great reviewer for the xpcom/ patches, so perhaps you might be able to trick him into reviewing this, too.

[0]: http://logs.glob.uno/?c=mozilla%23content&s=4+Feb+2015&e=4+Feb+2015#c263603
Flags: needinfo?(bugs)
Another directory that would be nice to convert would be embedding/browser.  This code is really old with some really awful style (there was 3 space indenting!), but has recently become important because it is used to control content processes on b2g and e10s.  I've started fixing up the style in bug 1064439 and bug 1127430, but I'm sure there's more that could be done.  There are only a few C++ files.

I could review style changes if that's okay with Olli and he doesn't want to do it.
I think converting docshell to gecko style would be great.
And you can trick Andrew to review the patch :p
Flags: needinfo?(bugs)
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Another directory that would be nice to convert would be embedding/browser. 
> This code is really old with some really awful style (there was 3 space
> indenting!), but has recently become important because it is used to control
> content processes on b2g and e10s.

Filed bug 1130096. If you have other directories in mind, please let me know (or file a bug and CC me).

(In reply to Andrew McCreight [:mccr8] from comment #1)
> I could review style changes if that's okay with Olli and he doesn't want to
> do it.

Great!
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1129795/poiru (obsolete) —
/r/3529 - Bug 1129795 - Fix indentation in docshell/base/*.h
/r/3531 - Bug 1129795 - Fix style in docshell/base/*.h
/r/3533 - Bug 1129795 - Fix argument names in docshell/base/*.h
/r/3535 - Bug 1129795 - Fix indentation in docshell/base/*.cpp
/r/3537 - Bug 1129795 - Fix style in docshell/base/*.cpp
/r/3539 - Bug 1129795 - Fix argument names in docshell/base/*.cpp

Pull down these commits:

hg pull review -r da2733a33052240ebe5be42d37d973ff8cbc2d95
Attachment #8561016 - Flags: review?(continuation)
Please make sure the files you fixed up have the standard mode lines at the top. (from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line ).

If you could rename DropDocShellreference() to DropDocShellReference() that would be good.

In the declaration of CloneAndReplaceChild, the last two arguments are on the same line, whereas the rest are on their own line.  They should either all be on their own line, or put more than one argument on its own line earlier.  Same with SetChildHistoryEntry.

usec_per_sec in PRTimeToSeconds should be usecPerSec or something.

In nsDSURIContentListener::CheckOneFrameOptionsPolicy, the indenting on the second line of the declaration of thisDocShellItem looks wrong. Though that whole line is horribly long so I'm not sure what the right thing to do is.  Also, right after that, please split up parentDocShellItem and currDocShellItem into two declarations.

In nsDefaultURIFixup::ConvertFileToStringURI: I think the second line of |if(kNotFound != aIn.FindChar('\\') ||| needs to be indented one less.

In nsDocShell::Init() the line after "new InterfaceRequestorProxy(static_cast<nsIInterfaceRequestor*>" seems indented incorrectly.

In nsDocShell::GetInterface, the code around "return NS_SUCCEEDED(" then "GetAuthPrompt(PROMPT_NORMAL, aIID, aSink)) ?" is indented weirdly.
The definition of the SAFE_ESCAPE is being indented by 4 instead of 2.

In nsDocShell::OnRedirectStateChange, the line after  "appCacheChannel->SetChooseApplicationCache(NS_ShouldCheckAppCache(principal," is not properly indented.

Rename the local variable bIsJavascript to isJavascript.
In kRedirMap in docshell/base/nsAboutRedirector.cpp for the "Fix style in docshell/base/*.cpp" patch: You should get Olli to review those changes.  This little array table thing is kind of stylized, so maybe it makes sense to leave it as it is.

I'll review more of that patch tomorrow.
In nsAboutRedirector::Create, you could just get rid of the null check on the return value of new.  Likewise in NS_NewURIFixup.

In nsDefaultURIFixup::FileURIFixup, when you put nsresult on its own line, that broke the indentation for the second argument.

You may want to remove the comment "// Note: operator new zeros our memory" before the nsDocShell ctor as I don't think that's accurate in general.

In nsDocShell::DisplayLoadError, near "case NS_ERROR_REMOTE_XUL", just remove the braces rather than fixing them up.

In nsDocShell::EndPageLoad, there's an else if that should be tucked up near the comment "Errors to be shown for any frame".  Maybe the comment could go into the body of that if.  Likewise with the else if near the comment "if we have a host".

In nsDocShell::CreateContentViewer, I think the cast in "NS_ENSURE_SUCCESS(Embed(viewer, "", (nsISupports*)nullptr)," isn't needed.

In nsDocShell::AddToSessionHistory, "expTime <=  now" has an extra space after the "<=".

In nsDocShell::EnsureTransferableHookData(), you can just delete the null check of the return value of new rather than reformat it.

In the comment right before nsDocShell::ReloadDocument, the "i" from the first "is" somehow got dropped.

That's it for the second to last patch.  I'll try to get to the last one tomorrow.

Thanks for all of your work here!
For the final patch:

In nsDSURIContentListener::DoContent, "aRequest" should be left as "request" in this comment:
// In case of multipart jpeg aRequest (mjpeg) we don't really want to

In nsDefaultURIFixupInfo::GetOriginalInput: it looks like aInput should be renamed to aResult.

docshell/base/nsDocShell.cpp:

In ForEachPing, "aContent" should be left as "content" in this comment:
      just parse the raw attribute.  It might be nice if the aContent node

In SendPing, "aURI" should be left as "uri" in this comment:
// Make sure the referrer and the given aURI share the same origin.  We

It is nice to see the standardization of names of these return arguments.

In nsDocShell::SetSize and nsDocShell::GetSize, it looks like aCY should be renamed to aHeight.
The MozReview diff was much easier to review than Splinter, so thanks for that.  Sorry I didn't put my comments inline using it, but I didn't feel comfortable entering my bugzilla password there.
Comment on attachment 8561016 [details]
MozReview Request: bz://1129795/poiru

Olli, in the "Fix style in docshell/base/*.cpp" patch, please review the changes to kRedirMap in docshell/base/nsAboutRedirector.cpp.  It kind of seems like it would be better as is, but I'm not familiar with this code, obviously.  (To get over to the MozReview patches, just click on the "MozReview Request: bz://1129795/poiru" attachment.)
Attachment #8561016 - Flags: review?(continuation)
Attachment #8561016 - Flags: review?(bugs)
Attachment #8561016 - Flags: review+
Why would kRedirMap be better as it is now? The patch makes it follow the coding style, and 2 spaces indentation is fine there. Or am I missing something.
Comment on attachment 8561016 [details]
MozReview Request: bz://1129795/poiru

(I downloaded the patch and looked at it in an editor. Don't feel comfortable with MozReview yet, and never use diff or splinter in bugzilla anyway)
Attachment #8561016 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #12)
> Why would kRedirMap be better as it is now? The patch makes it follow the
> coding style, and 2 spaces indentation is fine there. Or am I missing
> something.

It is just a table of data, so the existing format lets you see things broken into columns a little better, was my thinking.  But if you prefer it formatted correctly that is fine with me.
Attachment #8561016 - Flags: review?(continuation)
Attachment #8561016 - Flags: review+
Comment on attachment 8561016 [details]
MozReview Request: bz://1129795/poiru

/r/3529 - Bug 1129795 - Fix indentation in rest of docshell/
/r/3531 - Bug 1129795 - Fix style in rest of docshell/
/r/3533 - Bug 1129795 - Fix argument names in rest of docshell/

Pull down these commits:

hg pull review -r 4a7fa4d551bb0f7622e1f9fd06c7c3c7534d8c09
https://reviewboard.mozilla.org/r/3531/#review4097

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -//***    nsSHistory: Object Management
> +  : mListRoot(nullptr)

mListRoot is a COMPtr so you can just remove that line entirely.

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -  while(1) {
> +  while (1) {

true instead of 1 maybe?

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -  if (!canGoBack)  // Can't go back
> +  if (!canGoBack) {

Yikes, that's quite the useless comment.

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -  int32_t pCount=0, nCount=0;
> +  int32_t pCount = 0, nCount = 0;

While you are here, please split this into two declarations.

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -    }   // (pCount >0)
> +    }

The else on the next line should also get tucked in.

::: docshell/shistory/src/nsSHistory.cpp
(Diff revision 2)
> -  int32_t pcnt=0, ncnt=0, dsCount=0;
> +  int32_t pcnt = 0, ncnt = 0, dsCount = 0;

Please split this into separate declarations.
The first patch looked fine to me.
https://reviewboard.mozilla.org/r/3533/#review4111

::: docshell/shistory/src/nsSHEntry.cpp
(Diff revision 2)
>    mShared = new nsSHEntryShared();

If you could initialize mShared in the initializer list, maybe in the previous patch, that would be nice.
Comment on attachment 8561016 [details]
MozReview Request: bz://1129795/poiru

Thanks!
Attachment #8561016 - Flags: review?(continuation) → review+
I wrote some scripts to fix mode lines, so once this lands I can just fix that myself (see bug 1151541).
Are you going to have a chance to land whatever remains here soon?
Flags: needinfo?(birunthan)
(In reply to Andrew McCreight [:mccr8] from comment #23)
> Are you going to have a chance to land whatever remains here soon?

Yep, I'll land this later today. Sorry for the delay.
Flags: needinfo?(birunthan)
(In reply to Birunthan Mohanathas [:poiru] from comment #24)
> Yep, I'll land this later today. Sorry for the delay.

There's no hurry. I just wanted to make sure it hadn't gotten lost in the shuffle.
https://hg.mozilla.org/mozilla-central/rev/db70fd907041
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8561016 - Attachment is obsolete: true
Attachment #8619327 - Flags: review+
Attachment #8619328 - Flags: review+
Attachment #8619329 - Flags: review+
Attachment #8619330 - Flags: review+
Attachment #8619331 - Flags: review+
Attachment #8619332 - Flags: review+
You need to log in before you can comment on or make changes to this bug.