Bug 184424 - Expand WebCore policy concept of "shouldContinue" to allow for more than true/false
Summary: Expand WebCore policy concept of "shouldContinue" to allow for more than true...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-09 12:24 PDT by Brady Eidson
Modified: 2018-04-09 17:32 PDT (History)
8 users (show)

See Also:

Attachments
Patch (21.24 KB, patch)
2018-04-09 12:28 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (21.25 KB, patch)
2018-04-09 13:42 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (21.10 KB, patch)
2018-04-09 15:29 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (21.34 KB, patch)
2018-04-09 15:54 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2018-04-09 12:24:33 PDT 
Expand WebCore policy concept of "shouldContinue" to allow for more than true/false

Specifically:
-Yes
-No
-ForSuspension
Comment 1 Brady Eidson 2018-04-09 12:28:16 PDT Comment hidden (obsolete)
Comment 2 Brady Eidson 2018-04-09 13:42:31 PDT 
Created attachment 337536 [details]
Patch
Comment 3 Alex Christensen 2018-04-09 13:56:28 PDT 
Comment on attachment 337536 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3186
> +        FALLTHROUGH;

I don't think this FALLTHROUGH is necessary here or above.
I also think this whole thing could be just shouldContinue != ShouldContinue::No

> Source/WebCore/loader/PolicyChecker.h:60
> +using NavigationPolicyDecisionFunction = WTF::CompletionHandler<void(ResourceRequest&&, FormState*, ShouldContinue)>;

WTF:: could be removed.
Comment 4 Brady Eidson 2018-04-09 14:30:28 PDT 
(In reply to Alex Christensen from comment #3)
> Comment on attachment 337536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337536&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:3186
> > +        FALLTHROUGH;
> 
> I don't think this FALLTHROUGH is necessary here or above.
> I also think this whole thing could be just shouldContinue !=
> ShouldContinue::No

But I want to make sure all cases are always accounted for even if new ones are addddeddddddd!!!!


> 
> > Source/WebCore/loader/PolicyChecker.h:60
> > +using NavigationPolicyDecisionFunction = WTF::CompletionHandler<void(ResourceRequest&&, FormState*, ShouldContinue)>;
> 
> WTF:: could be removed.

Sure
Comment 5 Alex Christensen 2018-04-09 15:15:21 PDT 
Comment on attachment 337536 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3190
> +    case ShouldContinue::ForSuspension:
> +        shouldContinueBool = true;
> +    case ShouldContinue::No:
> +        shouldContinueBool = false;

Give me a break!
Comment 6 Brady Eidson 2018-04-09 15:29:23 PDT 
Created attachment 337554 [details]
Patch
Comment 7 Brady Eidson 2018-04-09 15:30:19 PDT 
(In reply to Alex Christensen from comment #5)
> Comment on attachment 337536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337536&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:3190
> > +    case ShouldContinue::ForSuspension:
> > +        shouldContinueBool = true;
> > +    case ShouldContinue::No:
> > +        shouldContinueBool = false;
> 
> Give me a break!

No!

(Okay maybe)
Comment 8 Brady Eidson 2018-04-09 15:54:49 PDT 
Created attachment 337557 [details]
Patch
Comment 9 WebKit Commit Bot 2018-04-09 17:31:08 PDT 
Comment on attachment 337557 [details]
Patch

Clearing flags on attachment: 337557

Committed r230458: <https://trac.webkit.org/changeset/230458>
Comment 10 WebKit Commit Bot 2018-04-09 17:31:09 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-04-09 17:32:19 PDT 
<rdar://problem/39299789>