Closed Bug 1406161 Opened 7 years ago Closed 6 years ago

Consider organizing methods & fields on nsDocShell

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: nika, Assigned: freesamael)

References

Details

Attachments

(6 files)

Bug 1406161 - Part 1: Reorder #include, #define, forward declarations, global & static variables.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1406161 - Part 2: Reorder docshell members and remove unnecessary `virtual` or `NS_IMETHOD` keywords.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1406161 - Part 3: Remove a dead member function.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1406161 - Part 4: Move nsPingListener and nsRefreshTimer to separate files.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1406161 - Part 5: Move LoadType convertion functions to nsDocShellLoadTypes.h.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1406161 - Part 6: Move shistory related static functions to nsSHistory.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
Currently the methods and fields on nsDocShell are distributed throughout the class declaration. For example, there are multiple different protected, private and public blocks in the class, and there are many members which are placed in the middle of the declaration.

It might make the class easier to understand if we sort these fields such that they are grouped based on functionality, and there is only a single protected, private, and public block in the type. 

It might also make sense to make all protected methods and fields private, as nsDocShell is final, so (I think) they mean the same thing.
The biggest downsides I can think of for doing this are:
a) code churn - any patches which modify nsDocShell.h will probably need rebasing after this change, and
b) blame pollution - which will be mitigated by the fact that only the header would be changed - the cpp file would still remain a 16k line long disaster (I have no intention of sorting that)

The main advantage is that it would make figuring out where to put methods during the window-divorce project easier (right now I'm just sorta throwing them in, 'cause there doesn't seem to be much organization.).

bz/smaug, do you have any feelings about whether or not this is worthwhile to do?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
I think this would be worthwhile, yes.  We could even consider moving things to sub-objects, if we identify sets of members coupled to each other but decoupled from everything else...
Flags: needinfo?(bzbarsky)
yeah, I think whatever makes nsDocShell code easier to follow is worthwhile.
Flags: needinfo?(bugs)
Priority: -- → P3
Assignee: nobody → sawang
Comment on attachment 8937204 [details]
Bug 1406161 - Part 1: Reorder #include, #define, forward declarations, global & static variables.

https://reviewboard.mozilla.org/r/207918/#review213906

rs+
Attachment #8937204 - Flags: review?(bugs) → review+
Comment on attachment 8937205 [details]
Bug 1406161 - Part 2: Reorder docshell members and remove unnecessary `virtual` or `NS_IMETHOD` keywords.

https://reviewboard.mozilla.org/r/207920/#review213908

rs+
Attachment #8937205 - Flags: review?(bugs) → review+
Comment on attachment 8937206 [details]
Bug 1406161 - Part 3: Remove a dead member function.

https://reviewboard.mozilla.org/r/207922/#review213912
Attachment #8937206 - Flags: review?(bugs) → review+
Comment on attachment 8937671 [details]
Bug 1406161 - Part 4: Move nsPingListener and nsRefreshTimer to separate files.

https://reviewboard.mozilla.org/r/208382/#review214140

::: docshell/base/nsDocShell.cpp:14032
(Diff revision 1)
>                           aIsTrusted, aTriggeringPrincipal);
>    return DispatchToTabGroup(TaskCategory::UI, ev.forget());
>  }
>  
> +static bool
> +IsElementAnchor(nsIContent* aContent)

Not about this bug, but I wish this could be renamed to IsElementAnchorOrArea.
r+ for a patch doing that.
Attachment #8937671 - Flags: review?(bugs) → review+
Comment on attachment 8937672 [details]
Bug 1406161 - Part 5: Move LoadType convertion functions to nsDocShellLoadTypes.h.

https://reviewboard.mozilla.org/r/208384/#review214148

Would be nice to avoid even tiniest possible code changes in code move patches.
Attachment #8937672 - Flags: review?(bugs) → review+
...but the patch is fine. The small changes in code move patches just make reviewing slower.
Comment on attachment 8937673 [details]
Bug 1406161 - Part 6: Move shistory related static functions to nsSHistory.

https://reviewboard.mozilla.org/r/208386/#review214156

(if I've misunderstood why you want to do it this way, explain and ask review again.)

::: commit-message-d7f90:1
(Diff revision 2)
> +Bug 1406161 - Part 6: Move frame history related static functions to a separate file. r?smaug

I don't really know what is frame history.
Should the file be called SessionHistoryUtils and then it could be in docshell/shistory.

Or possibly just add static methods to nsSHistory? I think this might be the easiest to understand approach.
Attachment #8937673 - Flags: review?(bugs) → review-
Comment on attachment 8937673 [details]
Bug 1406161 - Part 6: Move shistory related static functions to nsSHistory.

https://reviewboard.mozilla.org/r/208386/#review214156

> I don't really know what is frame history.
> Should the file be called SessionHistoryUtils and then it could be in docshell/shistory.
> 
> Or possibly just add static methods to nsSHistory? I think this might be the easiest to understand approach.

I'm just really bad at naming. Moved to nsSHistory.
Comment on attachment 8937204 [details]
Bug 1406161 - Part 1: Reorder #include, #define, forward declarations, global & static variables.

https://reviewboard.mozilla.org/r/207918/#review215326


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: docshell/base/nsDocShell.cpp:272
(Diff revision 2)
>  const char kAppstringsBundleURL[] = "chrome://global/locale/appstrings.properties";
>  
> +bool nsDocShell::sUseErrorPages = false;
> +
> +// Global reference to the URI fixup service.
> +nsIURIFixup* nsDocShell::sURIFixup = 0;

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

nsIURIFixup* nsDocShell::sURIFixup = 0;
                                     ^
                                     nullptr
Comment on attachment 8937673 [details]
Bug 1406161 - Part 6: Move shistory related static functions to nsSHistory.

https://reviewboard.mozilla.org/r/208386/#review215362

::: docshell/shistory/nsSHistory.cpp:426
(Diff revision 3)
> +    nsDocShell* childShell = nullptr;
> +    if (aRootShell) {
> +      // Walk the children of aRootShell and see if one of them
> +      // has srcChild as a SHEntry.
> +      int32_t length;
> +      aRootShell->GetChildCount(&length);

Tiny bit annoying to see code changes in a patch which is otherwise just move ;)
But I guess using GetChildAt is fine here. The old iterator is perhaps a tad faster, but this isn't performance critical code in any way.
Attachment #8937673 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #20)
> Would be nice to avoid even tiniest possible code changes in code move
> patches.

(In reply to Olli Pettay [:smaug] from comment #28)
> Tiny bit annoying to see code changes in a patch which is otherwise just
> move ;)

I'll try to split the commits in a better way next time.

> But I guess using GetChildAt is fine here. The old iterator is perhaps a tad
> faster, but this isn't performance critical code in any way.

I thought of adding a GetChildrenIterator or so at nsDocLoader, but I'm seeing operations tho nsIDocShellTreeItem everywhere.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7a1d967626f4 -d ba79a634141b: rebasing 440996:7a1d967626f4 "Bug 1406161 - Part 1: Reorder #include, #define, forward declarations, global & static variables. r=smaug"
merging docshell/base/nsDocShell.cpp
rebasing 440997:ff9f05c58e03 "Bug 1406161 - Part 2: Reorder docshell members and remove unnecessary `virtual` or `NS_IMETHOD` keywords. r=smaug"
merging docshell/base/nsDocShell.cpp
rebasing 440998:ae5d83aa0751 "Bug 1406161 - Part 3: Remove a dead member function. r=smaug"
merging docshell/base/nsDocShell.cpp
rebasing 440999:822dd5370ad6 "Bug 1406161 - Part 4: Move nsPingListener and nsRefreshTimer to separate files. r=smaug"
merging docshell/base/nsDocShell.cpp
warning: conflicts while merging docshell/base/nsDocShell.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased on m-c. Let's try it again.
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3165c7f9db20
Part 1: Reorder #include, #define, forward declarations, global & static variables. r=smaug
https://hg.mozilla.org/integration/autoland/rev/33cb29987d20
Part 2: Reorder docshell members and remove unnecessary `virtual` or `NS_IMETHOD` keywords. r=smaug
https://hg.mozilla.org/integration/autoland/rev/82763191a3c0
Part 3: Remove a dead member function. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ec9ea72e98dd
Part 4: Move nsPingListener and nsRefreshTimer to separate files. r=smaug
https://hg.mozilla.org/integration/autoland/rev/62a61710cf86
Part 5: Move LoadType convertion functions to nsDocShellLoadTypes.h. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4a7086f02b60
Part 6: Move shistory related static functions to nsSHistory. r=smaug
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: