Rewriting YouTube Flash embeds

Rewriting YouTube Flash embeds

BUG=625984

[kdsilva, 2016-07-11 15:36:15]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 15:36:30]

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-07-11 17:02:07]

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

[commit-bot: I haz the power, 2016-07-11 17:02:08]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-07-12 13:56:08]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2016-07-12 13:56:09]

I started going through this and adding nits, but stepping back a moment, this
seems like the wrong layer to add the feature you're interested in adding.

Blink should not know about YouTube as opposed to any other site. This patch is
probably fine for local experimentation, but I would suggest that code we'd
actually land would need to live at a higher layer of the system. That is,
//chrome could reasonably make this kind of policy decision about the way it
handles particular URLs (perhaps by hooking into the network stack and
redirecting requests with a `URLRequestInterceptor`). Or Blink could allow its
embedder to make a policy decision about a URL via a new platform API, or etc.
Any of those options seem preferable to hard-coding YouTube's URL structure into
Blink. That isn't an approach I'd like to see us take.

https://codereview.chromium.org/2132403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right):

https://codereview.chromium.org/2132403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:151: KURL url =
KURL(ParsedURLStringTag(), m_url);
Nit: `s/ParsedURLStringTag()/ParsedURLString/`.

Nit: You should probably be using `document().completeURL(m_url)` rather than
assuming that the URL is absolute.

https://codereview.chromium.org/2132403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLEmbedElement.h (right):

https://codereview.chromium.org/2132403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLEmbedElement.h:44: bool
rewriteYouTubeFlashEmbeds(String const & baseUrl);
Nit: `const String&`

Nit: `host` is probably a better name than `baseUrl`, as this isn't a URL.

[Avi (use Gerrit), 2016-07-12 14:12:21]

On 2016/07/12 13:56:09, Mike West wrote:
> I started going through this and adding nits, but stepping back a moment, this
> seems like the wrong layer to add the feature you're interested in adding.

Also, this is definitely the wrong place to be having this discussion. A change
like this requires a BUG attached to it. Please file a bug on new.crbug.com and
attach it here, so that we can have this discussion there.

[kdsilva, 2016-07-12 14:31:12]

Description was changed from

==========
Rewriting YouTube Flash embeds

BUG=
==========

to

==========
Rewriting YouTube Flash embeds

BUG=625984
==========

[whywhat, 2016-07-12 15:49:33]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2016-07-12 15:49:33]

To clarify, this is a prototype/experimental patch. We have a bug filed and now
referenced in the description (with zero discussion on the implementation there
yet) and a draft design doc to share with a broader audience.

The idea is to:

1. have an experimental patch that implements the functionality
2. add implementation-agnostic tests for various edge cases
3. once the tests cover our bases, iterate on the patch until it can be landed

Mike, the question was solely about how to do #2 (e.g. is there a way in layout
tests to check if the network request was modified or should it be a browser
test and so on), sorry it wasn't clear. Krittika intended to ask neither for
nits or even high level review at this point :)