Closed
Bug 1129795
Opened 9 years ago
Closed 9 years ago
Convert docshell/ to Gecko style
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: poiru, Assigned: poiru)
References
Details
Attachments
(6 files, 1 obsolete file)
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
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)
Comment 1•9 years ago
|
||
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.
Comment 2•9 years ago
|
||
I think converting docshell to gecko style would be great. And you can trick Andrew to review the patch :p
Flags: needinfo?(bugs)
Assignee | ||
Comment 3•9 years ago
|
||
(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
Assignee | ||
Comment 4•9 years ago
|
||
/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)
Comment 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
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.
Comment 8•9 years ago
|
||
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!
Comment 9•9 years ago
|
||
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.
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
(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.
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1641a146787f https://hg.mozilla.org/integration/mozilla-inbound/rev/6100bd3192d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c070487cfcb https://treeherder.mozilla.org/#/jobs?repo=try&revision=855002dd12a6
Keywords: leave-open
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1641a146787f https://hg.mozilla.org/mozilla-central/rev/6100bd3192d2 https://hg.mozilla.org/mozilla-central/rev/1c070487cfcb
Assignee | ||
Updated•9 years ago
|
Attachment #8561016 -
Flags: review?(continuation)
Attachment #8561016 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
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
Comment 18•9 years ago
|
||
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.
Comment 19•9 years ago
|
||
The first patch looked fine to me.
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
Comment on attachment 8561016 [details]
MozReview Request: bz://1129795/poiru
Thanks!
Attachment #8561016 -
Flags: review?(continuation) → review+
Comment 22•9 years ago
|
||
I wrote some scripts to fix mode lines, so once this lands I can just fix that myself (see bug 1151541).
Comment 23•9 years ago
|
||
Are you going to have a chance to land whatever remains here soon?
Flags: needinfo?(birunthan)
Assignee | ||
Comment 24•9 years ago
|
||
(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)
Comment 25•9 years ago
|
||
(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.
Assignee | ||
Comment 27•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7e7ce98406
Keywords: leave-open
Comment 28•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db70fd907041
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 29•9 years ago
|
||
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+
Assignee | ||
Comment 30•9 years ago
|
||
Assignee | ||
Comment 31•9 years ago
|
||
Assignee | ||
Comment 32•9 years ago
|
||
Assignee | ||
Comment 33•9 years ago
|
||
Assignee | ||
Comment 34•9 years ago
|
||
Assignee | ||
Comment 35•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•