Bug 173484 – NavigationAction has too many constructors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

We consider NavigationAction an immutable object once constructed. There are 12 different constructors for instantiating a NavigationAction. To have NavigationAction require more data on construction can require updating 11 of these constructors (-1 for the default constructor) or adding a new constructor. I suggest that we extract all the optional arguments passed to NavigationAction to a struct that is passed to a NavigationAction. This will allow us to reduce the number of constructors from 12 to 2: the default constructor and a constructor that takes a resource request and a struct of details.

[Daniel Bates]

Created attachment 313095 [details]
Patch

[Build Bot]

Attachment 313095 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 313103 [details]
Patch

[Build Bot]

Attachment 313103 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 313104 [details]
Patch

Attempt to fix the Windows EWS.

[Build Bot]

Attachment 313104 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 313149 [details]
Patch

Another attempt to fix the Windows EWS.

[Build Bot]

Attachment 313149 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Could we not achieve pretty much the same thing by using default values for the parameters?

[Daniel Bates]

(In reply to Sam Weinig from comment #9)
> Could we not achieve pretty much the same thing by using default values for
> the parameters?

We can. Will update patch to take this approach.

[Daniel Bates]

Created attachment 313188 [details]
Patch

[Build Bot]

Attachment 313188 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 313188 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313188&action=review

Stylistic comments only.

> Source/WebCore/loader/FrameLoader.cpp:3411
> -            action = NavigationAction(request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy);
> +            action = NavigationAction { request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy };

"action" is already known to be a NavigationAction object, and therefore the initializer list does not need a type name preceding it.

> Source/WebCore/loader/FrameLoader.cpp:3414
> +            action = NavigationAction { request, NavigationType::FormResubmitted, shouldOpenExternalURLsPolicy, event };

Ditto

> Source/WebCore/loader/FrameLoader.cpp:3703
> +    Page* page = oldPage->chrome().createWindow(openerFrame, requestWithReferrer, features, NavigationAction { requestWithReferrer.resourceRequest(), NavigationType::Other, shouldOpenExternalURLsPolicy });

Ditto

> Source/WebCore/loader/NavigationAction.cpp:47
> +NavigationAction::NavigationAction(const ResourceRequest& resourceRequest, NavigationType type,
> +    ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)
> +    : m_resourceRequest { resourceRequest }

We don't normally break up lines like this (the previous line wasn't nearly too long, IMHO), and this is confusing style-bot on like 47

[Daniel Bates]

Comment on attachment 313188 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313188&action=review

>> Source/WebCore/loader/FrameLoader.cpp:3411
>> +            action = NavigationAction { request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy };
> 
> "action" is already known to be a NavigationAction object, and therefore the initializer list does not need a type name preceding it.

We need to specify the type here because the NavigationAction constructor is marked explicit. It is marked explicit because in the case that only a ResourceRequest object is used to instantiate a NavigationAction it seems error prone to allow implicit conversion to NavigationAction. We could separate out the NavigationAction constructor that takes only a ResourceRequest object as an argument and mark that constructor explicit and remove the keyword explicit from the NavigationAction constructor overload that takes more than one argument.

>> Source/WebCore/loader/NavigationAction.cpp:47
>> +    : m_resourceRequest { resourceRequest }
> 
> We don't normally break up lines like this (the previous line wasn't nearly too long, IMHO), and this is confusing style-bot on like 47

Will unwrap this line before landing. We should fix the style checker :)

[Daniel Bates]

Created attachment 313341 [details]
Patch for landing

For EWS

[Build Bot]

Attachment 313341 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/NavigationAction.cpp:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to Daniel Bates from comment #14)
> Comment on attachment 313188 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313188&action=review
> 
> >> Source/WebCore/loader/FrameLoader.cpp:3411
> >> +            action = NavigationAction { request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy };
> > 
> > "action" is already known to be a NavigationAction object, and therefore the initializer list does not need a type name preceding it.
> 
> We need to specify the type here because the NavigationAction constructor is
> marked explicit. It is marked explicit because in the case that only a
> ResourceRequest object is used to instantiate a NavigationAction it seems
> error prone to allow implicit conversion to NavigationAction. We could
> separate out the NavigationAction constructor that takes only a
> ResourceRequest object as an argument and mark that constructor explicit and
> remove the keyword explicit from the NavigationAction constructor overload
> that takes more than one argument.

It's fine to have an explicit one - I would request *not* using initializer list construction in that case - That's very uncommon in the code base (at least in the parts where I spend my time, like the loader)

[Daniel Bates]

(In reply to Brady Eidson from comment #17)
> (In reply to Daniel Bates from comment #14)
> > Comment on attachment 313188 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313188&action=review
> > 
> > >> Source/WebCore/loader/FrameLoader.cpp:3411
> > >> +            action = NavigationAction { request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy };
> > > 
> > > "action" is already known to be a NavigationAction object, and therefore the initializer list does not need a type name preceding it.
> > 
> > We need to specify the type here because the NavigationAction constructor is
> > marked explicit. It is marked explicit because in the case that only a
> > ResourceRequest object is used to instantiate a NavigationAction it seems
> > error prone to allow implicit conversion to NavigationAction. We could
> > separate out the NavigationAction constructor that takes only a
> > ResourceRequest object as an argument and mark that constructor explicit and
> > remove the keyword explicit from the NavigationAction constructor overload
> > that takes more than one argument.
> 
> It's fine to have an explicit one - I would request *not* using initializer
> list construction in that case - That's very uncommon in the code base (at
> least in the parts where I spend my time, like the loader)

OK. Will keep the explicit NavigationAction constructor that has default values for all of its arguments and will use function style notation instead of C++11 brace initialization.

[Daniel Bates]

(In reply to Daniel Bates from comment #18)
> (In reply to Brady Eidson from comment #17)
> > (In reply to Daniel Bates from comment #14)
> > > Comment on attachment 313188 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=313188&action=review
> > > 
> > > >> Source/WebCore/loader/FrameLoader.cpp:3411
> > > >> +            action = NavigationAction { request, loadType, isFormSubmission, event, shouldOpenExternalURLsPolicy };
> > > > 
> > > > "action" is already known to be a NavigationAction object, and therefore the initializer list does not need a type name preceding it.
> > > 
> > > We need to specify the type here because the NavigationAction constructor is
> > > marked explicit. It is marked explicit because in the case that only a
> > > ResourceRequest object is used to instantiate a NavigationAction it seems
> > > error prone to allow implicit conversion to NavigationAction. We could
> > > separate out the NavigationAction constructor that takes only a
> > > ResourceRequest object as an argument and mark that constructor explicit and
> > > remove the keyword explicit from the NavigationAction constructor overload
> > > that takes more than one argument.
> > 
> > It's fine to have an explicit one - I would request *not* using initializer
> > list construction in that case - That's very uncommon in the code base (at
> > least in the parts where I spend my time, like the loader)
> 
> OK. Will keep the explicit NavigationAction constructor that has default
> values for all of its arguments and will use function style notation instead
> of C++11 brace initialization.

From quickly greping our source code using C++11 brace initialization syntax with an explicit type (e.g. NavigationAction { ... }) I see over 200 places in WebCore where we use this style.

I used the following grep command executed inside my top-level WebKit checkout:

grep -R -E -e '(\(|= )\w+ \{' --exclude 'ChangeLog*' --exclude '*.pl' --exclude '*.rb' --exclude-dir Source/WebCore/bindings Source/WebCore | wc -l

You can modify this query to see that we also use this syntax in WTF, JavaScriptCore, WebKit, and WebKit2.

[Daniel Bates]

Committed r218599: <http://trac.webkit.org/changeset/218599>

[Radar WebKit Bug Importer]

<rdar://problem/32884936>