Adds HTTP check to PresentationRequest creation.

Adds HTTP check to PresentationRequest creation.


BUG=670848
Review-Url: https://codereview.chromium.org/2566503002
Cr-Commit-Position: refs/heads/master@{#444586}
Committed: https://chromium.googlesource.com/chromium/src/+/a65f8aef002b29053fcd5ef52d118018c596d1c5

[CJ, 2016-12-08 21:20:02]

lethalantidote@chromium.org changed reviewers:
+ mfoltz@chromium.org, skonig@chromium.org

[CJ, 2016-12-08 21:20:02]

Let me know what you think.

[mark a. foltz, 2016-12-09 05:53:38]

mfoltz@chromium.org changed reviewers:
+ zhaobin@chromium.org
- skonig@chromium.org

[mark a. foltz, 2016-12-09 05:53:38]

Thanks, Looks good.

Can you update PresentationRequestTest to verify this?

Note, pending patch by zhaobin@ is also touching this code.
https://codereview.chromium.org/2552343009

[mark a. foltz, 2016-12-09 05:53:45]

lgtm

[zhaobin, 2016-12-09 18:53:02]

Thanks for doing this. Could you please double check it works fine for relative
url (I think it would be ok)?

This will cause merge conflict with my patch. Could you please let me know when
you land this? Thanks!

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:73: if
(!parsedUrl.isValid() || !parsedUrl.protocolIsInHTTPFamily() ||
Remove parsedUrl.protocolIsAbout() check if we already checked
protocolIsInHTTPFamily()?

[CJ, 2016-12-13 02:12:29]

On 2016/12/09 18:53:02, zhaobin wrote:
> Thanks for doing this. Could you please double check it works fine for
relative
> url (I think it would be ok)?
> 
> This will cause merge conflict with my patch. Could you please let me know
when
> you land this? Thanks!
> 
>
https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
> (right):
> 
>
https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:73: if
> (!parsedUrl.isValid() || !parsedUrl.protocolIsInHTTPFamily() ||
> Remove parsedUrl.protocolIsAbout() check if we already checked
> protocolIsInHTTPFamily()?

Hi, I am new to this area of the code. I don't see any test files... How does
one usually go about testing these sort of things?

[CJ, 2016-12-13 02:12:39]

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:73: if
(!parsedUrl.isValid() || !parsedUrl.protocolIsInHTTPFamily() ||
On 2016/12/09 18:53:02, zhaobin wrote:
> Remove parsedUrl.protocolIsAbout() check if we already checked
> protocolIsInHTTPFamily()?

I'm not seeing how they overlap with each other. Please explain?

[zhaobin, 2016-12-13 19:06:02]

Mark addes a unit test for PresentationRequest (not landed yet
https://codereview.chromium.org/2552343009/). You may wait for that or create a
unit test for PresentationRequest.

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:73: if
(!parsedUrl.isValid() || !parsedUrl.protocolIsInHTTPFamily() ||
On 2016/12/13 02:12:38, CJ wrote:
> On 2016/12/09 18:53:02, zhaobin wrote:
> > Remove parsedUrl.protocolIsAbout() check if we already checked
> > protocolIsInHTTPFamily()?
> 
> I'm not seeing how they overlap with each other. Please explain?

!protocolIsInHTTPFamily() means not http or https. about is not http or https,
so no need to check protocolIsAbout() again?

[CJ, 2016-12-13 21:34:23]

@mfoltz, please let me know when ur unittests land.

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:73: if
(!parsedUrl.isValid() || !parsedUrl.protocolIsInHTTPFamily() ||
On 2016/12/13 19:06:02, zhaobin wrote:
> On 2016/12/13 02:12:38, CJ wrote:
> > On 2016/12/09 18:53:02, zhaobin wrote:
> > > Remove parsedUrl.protocolIsAbout() check if we already checked
> > > protocolIsInHTTPFamily()?
> > 
> > I'm not seeing how they overlap with each other. Please explain?
> 
> !protocolIsInHTTPFamily() means not http or https. about is not http or https,
> so no need to check protocolIsAbout() again? 

Done.

[zhaobin, 2016-12-13 21:36:18]

lgtm

[CJ, 2016-12-14 23:54:19]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 23:54:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:42:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:42:58]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mark a. foltz, 2016-12-15 22:12:29]

The unit tests are landing as part of
https://codereview.chromium.org/2552343009.

[mark a. foltz, 2017-01-17 21:37:15]

On 2016/12/15 at 22:12:29, mark a. foltz wrote:
> The unit tests are landing as part of
https://codereview.chromium.org/2552343009.

https://codereview.chromium.org/2552343009/ just landed which adds
PresentationRequestTest.cpp.  If you rebase and add a unit test, I think this
will be ready to land.

[CJ, 2017-01-18 00:42:30]

On 2017/01/17 21:37:15, mark a. foltz wrote:
> On 2016/12/15 at 22:12:29, mark a. foltz wrote:
> > The unit tests are landing as part of
> https://codereview.chromium.org/2552343009.
> 
> https://codereview.chromium.org/2552343009/ just landed which adds
> PresentationRequestTest.cpp.  If you rebase and add a unit test, I think this
> will be ready to land.

PTAL

Added unittest.

[zhaobin, 2017-01-18 18:58:22]

Could you please verify that presentation layout tests still pass? Instruction
for layout tests:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t...

https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:50:
urls.append("cast://deadbeef?param=foo");
Some layout tests uses url with "cast://". They may break if we are rejecting
those urls now. Could you please double check?

src/third_party/WebKit/LayoutTests/presentation/presentation-start.html
src/third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html

[mark a. foltz, 2017-01-18 20:26:01]

https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp
(right):

https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:50:
urls.append("cast://deadbeef?param=foo");
On 2017/01/18 at 18:58:22, zhaobin wrote:
> Some layout tests uses url with "cast://". They may break if we are rejecting
those urls now. Could you please double check?
> 
> src/third_party/WebKit/LayoutTests/presentation/presentation-start.html
>
src/third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html

Yeah, now that we have tests for cast:// we should allow it in
PresentationRequest.cpp.

[CJ, 2017-01-18 23:38:38]

On 2017/01/18 20:26:01, mark a. foltz wrote:
>
https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2566503002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:50:
> urls.append("cast://deadbeef?param=foo");
> On 2017/01/18 at 18:58:22, zhaobin wrote:
> > Some layout tests uses url with "cast://". They may break if we are
rejecting
> those urls now. Could you please double check?
> > 
> > src/third_party/WebKit/LayoutTests/presentation/presentation-start.html
> >
>
src/third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
> 
> Yeah, now that we have tests for cast:// we should allow it in
> PresentationRequest.cpp.

Updated things so that it should work with cast:// and that the layouttests
work. PTAL

[CJ, 2017-01-18 23:38:45]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 23:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zhaobin, 2017-01-19 00:51:02]

lgtm

[zhaobin, 2017-01-19 00:51:06]

lgtm

[commit-bot: I haz the power, 2017-01-19 01:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 01:10:03]

Dry run: This issue passed the CQ dry run.

[CJ, 2017-01-19 01:15:29]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2017-01-19 01:15:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2566503002/#ps80001
(title: "Updates browser test.")

[commit-bot: I haz the power, 2017-01-19 01:16:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 01:21:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484788529887540,
"parent_rev": "c11dd43622d564f950c0d1f3e940507e32e25a87", "commit_rev":
"a65f8aef002b29053fcd5ef52d118018c596d1c5"}

[commit-bot: I haz the power, 2017-01-19 01:22:30]

Description was changed from

==========
Adds HTTP check to PresentationRequest creation.


BUG=670848
==========

to

==========
Adds HTTP check to PresentationRequest creation.


BUG=670848

Review-Url: https://codereview.chromium.org/2566503002
Cr-Commit-Position: refs/heads/master@{#444586}
Committed:
https://chromium.googlesource.com/chromium/src/+/a65f8aef002b29053fcd5ef52d11...
==========

[commit-bot: I haz the power, 2017-01-19 01:22:31]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a65f8aef002b29053fcd5ef52d11...