Bug 112483 - [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, WebStreamTextureClient, and WebVideoLayer classes.
Summary: [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, WebStreamTextureC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 112971
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-15 17:30 PDT by Dana Jansens
Modified: 2013-04-08 16:58 PDT (History)
17 users (show)

See Also:


Attachments
Patch (31.53 KB, patch)
2013-03-15 17:30 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (32.91 KB, patch)
2013-03-18 12:09 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (32.92 KB, patch)
2013-03-18 12:54 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (35.70 KB, patch)
2013-03-18 17:29 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (36.13 KB, patch)
2013-03-20 15:42 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (38.32 KB, patch)
2013-03-20 20:27 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (18.46 KB, patch)
2013-03-25 17:31 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (18.45 KB, patch)
2013-03-25 17:32 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (20.99 KB, patch)
2013-03-26 17:23 PDT, Dana Jansens
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff
Patch (18.51 KB, patch)
2013-03-26 17:36 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2013-03-15 17:30:16 PDT
[wip] [chromium] Remove the WebVideoFrame, WebVideoFrameProvider, and WebVideoLayer classes.
Comment 1 Dana Jansens 2013-03-15 17:30:38 PDT
Created attachment 193405 [details]
Patch
Comment 2 Dana Jansens 2013-03-15 17:31:43 PDT
This isn't for landing. This will have to land in pieces. But I'd like to do review on the final appearance before I split it up, to make sure I'm on the right page.
Comment 3 WebKit Review Bot 2013-03-15 17:43:07 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 WebKit Review Bot 2013-03-15 17:43:21 PDT
Attachment 193405 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/Platform.gypi', u'Source/Platform/chromium/public/WebCompositorSupport.h', u'Source/Platform/chromium/public/WebLayer.h', u'Source/Platform/chromium/public/WebVideoFrame.h', u'Source/Platform/chromium/public/WebVideoFrameProvider.h', u'Source/Platform/chromium/public/WebVideoLayer.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/WebKit.gypi', u'Source/WebKit/chromium/public/WebMediaPlayer.h', u'Source/WebKit/chromium/public/WebStreamTextureClient.h', u'Source/WebKit/chromium/public/WebVideoFrame.h', u'Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp', u'Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h', u'Source/WebKit/chromium/tests/WebMediaPlayerClientImplTest.cpp']" exit_code: 1
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 WebKit Review Bot 2013-03-15 17:45:51 PDT
Comment on attachment 193405 [details]
Patch

Attachment 193405 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17200298
Comment 6 WebKit Review Bot 2013-03-15 17:49:59 PDT
Comment on attachment 193405 [details]
Patch

Attachment 193405 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17189580
Comment 7 Peter Beverloo (cr-android ews) 2013-03-15 17:55:43 PDT
Comment on attachment 193405 [details]
Patch

Attachment 193405 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17234034
Comment 8 James Robinson 2013-03-17 18:51:23 PDT
Comment on attachment 193405 [details]
Patch

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

> Source/Platform/chromium/public/WebLayer.h:206
> +    // True if the layer is not attached to a LayerTreeHost.

There's no LayerTreeHost in the WebKit API and there's no real notion of 'attached'.  There is a WebLayerTreeView which has a WebLayer that's set as the root layer, so I think you could define this function in terms of that

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:100
> -#if USE(ACCELERATED_COMPOSITING)
> -    if (m_videoFrameProviderClient)
> -        m_videoFrameProviderClient->stopUsingProvider();
>      // No need for a lock here, as getCurrentFrame/putCurrentFrame can't be
>      // called now that the client is no longer using this provider. Also, load()
>      // and this destructor are called from the same thread.
>      if (m_webMediaPlayer)
>          m_webMediaPlayer->setStreamTextureClient(0);

How does the teardown path work in this new world?  We've had a lot of deadlocks/crashes around this logic in the past.
Comment 9 Dana Jansens 2013-03-18 12:09:42 PDT
Created attachment 193625 [details]
Patch
Comment 10 Dana Jansens 2013-03-18 12:11:13 PDT
Comment on attachment 193405 [details]
Patch

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

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:100
>>          m_webMediaPlayer->setStreamTextureClient(0);
> 
> How does the teardown path work in this new world?  We've had a lot of deadlocks/crashes around this logic in the past.

Thanks for this great question. I went through before/after of the teardown paths, and found some problems and missing mutexes and have cleaned this up. Here's the state of the world with patchset #2 (also updated the chromium side patch).

BEFORE - If the video layer is destroyed:

~VideoLayerImpl()
=> VideoFPClientImpl::Stop()
  => WebMediaPlayerClientImpl::setVideoFrameProviderClient(NULL)
    locks access to ~WebMediaPlayerImpl, setVFPClient, getCurrentFrame, putCurrentFrame.
    => VideoFPClientImpl::StopUsingProvider()
      sets provider_ = NULL; with provider_ lock.
    sets m_videoFrameProviderClient = NULL
  => WebMediaPlayerImpl::setStreamTextureClient(NULL)

AFTER - If the video layer is destroyed:

~VideoLayerImpl()
=> VideoFPClientImpl::Stop()
  => WebMediaPlayerImpl::SetVideoFrameProviderClient(NULL)
    locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL; with provider_lock.
  sets video_frame_provider_client_ = NULL
  => WebMediaPlayerImpl::setStreamTextureClient(NULL)

Then, when WebMediaPlayImpl later shuts down, it has no pointer to the video layer. And the layer has no pointer to the media player.

=================

BEFORE - If the WebMediaPlayerClientImpl is destroyed:

~WebMediaPlayerClientImpl()
=>VideoFPClientImpl::StopUsingProvider()
  sets provider_ = NULL; with provider_ lock.
=>WebMediaPlayImpl::SetStreamTextuerClient(NULL)
=>~WebMediaPlayerImpl()

AFTER - If the WebMediaPlayerClientImpl is destroyed, then

~WebMediaPlayerClientImpl()
=>~WebMediaPlayerImpl()
  =>SetVideoFrameProviderClient(NULL)
    locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL; with provider_lock.
  => WebMediaPlayerImpl::setStreamTextureClient(NULL)

========================

BEFORE - If the WebMediaPlayerClientImpl::loadRequested() destoys the media player.

WebMediaPlayerClientImpl::loadRequested()
  locks access to setVideoFrameProviderClient, getCurrentFrame, putCurrentFrame
  =>~WebMediaPlayerImpl()
  =>replace WebMediaPlayerImpl*

no need to replace VFProvider pointer.

AFTER - If the WebMediaPlayerClientImpl::loadRequested() destoys the media player.

WebMediaPlayerClientImpl::loadRequested()
=>~WebMediaPlayerImpl()
  =>SetVideoFrameProviderClient(NULL)
    locks access to ~WebMediaPlayerImpl, SetVideoFrameProviderClient
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL; with provider_lock.
  => WebMediaPlayerImpl::setStreamTextureClient(NULL)
>>>>>The video layer needs to be recreated with the new provider<<<<<
=>unRegister the video layer and destroy it.
=>replace WebMediaPlayerImpl*
Comment 11 Dana Jansens 2013-03-18 12:12:16 PDT
I'd like to verify the WedMediaPlayerClientImpl::loadRequested() path that recreates a new WebMediaPlayer works correctly, but I'm not exactly sure how to invoke it. Any test cases would be appreciated. @scherkus?
Comment 12 WebKit Review Bot 2013-03-18 12:31:21 PDT
Comment on attachment 193625 [details]
Patch

Attachment 193625 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17235073
Comment 13 WebKit Review Bot 2013-03-18 12:35:59 PDT
Comment on attachment 193625 [details]
Patch

Attachment 193625 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17114990
Comment 14 Peter Beverloo (cr-android ews) 2013-03-18 12:53:32 PDT
Comment on attachment 193625 [details]
Patch

Attachment 193625 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17172328
Comment 15 Dana Jansens 2013-03-18 12:54:38 PDT
Created attachment 193633 [details]
Patch

isOrphan => True if the layer is not part of a tree attached to a WebLayerTreeView.
Comment 16 WebKit Review Bot 2013-03-18 13:15:50 PDT
Comment on attachment 193633 [details]
Patch

Attachment 193633 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17116669
Comment 17 WebKit Review Bot 2013-03-18 13:28:20 PDT
Comment on attachment 193633 [details]
Patch

Attachment 193633 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17184358
Comment 18 Peter Beverloo (cr-android ews) 2013-03-18 13:51:49 PDT
Comment on attachment 193633 [details]
Patch

Attachment 193633 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17130616
Comment 19 Andrew Scherkus 2013-03-18 14:43:35 PDT
I'm not a WK pro by any means, but this combined with the Chromium CL looks good (in other words, I can't spot anything horribly wrong)
Comment 20 Dana Jansens 2013-03-18 17:29:03 PDT
Created attachment 193701 [details]
Patch

Remove WebStreamTextureClient
Comment 21 WebKit Review Bot 2013-03-18 17:45:43 PDT
Comment on attachment 193701 [details]
Patch

Attachment 193701 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17197481
Comment 22 WebKit Review Bot 2013-03-18 17:47:50 PDT
Comment on attachment 193701 [details]
Patch

Attachment 193701 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17231194
Comment 23 Peter Beverloo (cr-android ews) 2013-03-18 17:51:52 PDT
Comment on attachment 193701 [details]
Patch

Attachment 193701 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17197480
Comment 24 Min Qin 2013-03-18 19:45:42 PDT
if webStreamTextureClient is removed, we need a replacement strategy for streamTextureProxy to communicate with videolayerimpl when a video frame arrives.

(In reply to comment #20)
> Created an attachment (id=193701) [details]
> Patch
> 
> Remove WebStreamTextureClient
Comment 25 Dana Jansens 2013-03-18 19:54:56 PDT
(In reply to comment #24)
> if webStreamTextureClient is removed, we need a replacement strategy for streamTextureProxy to communicate with videolayerimpl when a video frame arrives.

Please take a look at https://codereview.chromium.org/12902002/. I have StreamTextureProxy going directly to cc::VideoFrameProvider::Client.
Comment 26 Dana Jansens 2013-03-18 20:48:35 PDT
here's an updated look at where we're at now that I have a better grasp of what's going on with stream texture and locking and whatnot:

BEFORE - If the video layer is destroyed:

~VideoLayerImpl()
=> VideoFPClientImpl::Stop()
  => WebMediaPlayerClientImpl::setVideoFrameProviderClient(NULL)
    locks m_videoFrameProviderClient
    => VideoFPClientImpl::StopUsingProvider()
      sets provider_ = NULL
    sets m_videoFrameProviderClient = NULL

AFTER - If the video layer is destroyed:

~VideoLayerImpl()
=> VideoFPClientImpl::Stop()
  => WebMediaPlayerImpl::SetVideoFrameProviderClient(NULL)
    locks video_frame_provider_client_
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL
  sets video_frame_provider_client_ = NULL

Then, when WebMediaPlayImpl later shuts down, it has no pointer to the video layer. And the layer has no pointer to the media player.

After the video layer is destroyed the compositor will not use the VFProvider that was attached to the layer anymore, so we don't need to worry about GetCurrentFrame() being called on the WMPImpl anymore.

The VideoLayerImpl ensures the main thread is blocked if it sets the VFPClient to NULL, so the lock in SetVideoFrameProviderClient is only there to ensure the value changed on the compositor thread is updated correctly on the main thread - not for blocking the main thread.

=================

BEFORE - If the WebMediaPlayerClientImpl is destroyed:

~WebMediaPlayerClientImpl()
=>VideoFPClientImpl::StopUsingProvider()
  sets provider_ = NULL
=>~WebMediaPlayerImpl()

AFTER - If the WebMediaPlayerClientImpl is destroyed, then

~WebMediaPlayerClientImpl()
=>~WebMediaPlayerImpl()
  =>SetVideoFrameProviderClient(NULL)
    locks video_frame_provider_client_
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL

This is as safe as before. Previously, access to the m_videoFrameProviderClient was not guarded by the m_webMediaPlayerMutex, which could cause a crash. But this was safe-guarded by compositor only setting the VideoFrameProviderClient to NULL when the main thread was blocked.

The video layer impl's connection to its VFProvider is broken still.

The VideoLayerImpl ensures the main thread is blocked if it sets the VFPClient to NULL, so the lock in SetVideoFrameProviderClient is only there to ensure the value changed on the main thread is updated correctly on the compositor thread - not for blocking the compositor thread.

========================

BEFORE - If the WebMediaPlayerClientImpl::loadRequested() destroys the media player.

WebMediaPlayerClientImpl::loadRequested()
  locks access to m_webMediaPlayer
  =>~WebMediaPlayerImpl()
  =>replace WebMediaPlayerImpl*

No need to replace VFProviderClient pointer since it points to the WMPClientImpl. Ensures that getCurrentFrame() does not happen while the WMPImpl is destroyed with the m_webMediaPlayerMutex.

AFTER - If the WebMediaPlayerClientImpl::loadRequested() destroys the media player.

WebMediaPlayerClientImpl::loadRequested()
=>~WebMediaPlayerImpl()
  =>SetVideoFrameProviderClient(NULL)
    locks video_frame_provider_client_
    => VideoFPClientImpl::StopUsingProvider()
    sets provider_ = NULL
>>>>>The video layer needs to be recreated with the new provider<<<<<
=>unRegister the video layer and destroy it.
=>replace WebMediaPlayerImpl*
>>>>>Later, the video layer will be recreated and pointed at the WMPImpl as its VFProviderClient<<<<<

The video layer impl's connection to its VFProvider (the WMPImpl) is broken before destroying the WMPImpl. If the compositor tries to GetCurrentFrame() afterward it will fail in VFPClientImpl. On the next commit the VideoLayerImpl will be destroyed.

Later, when the ready state changes, a new video layer is added to the compositing tree and pointed at the new WMPImpl which was created here.
Comment 27 James Robinson 2013-03-19 16:28:29 PDT
Comment on attachment 193701 [details]
Patch

I really like the direction this is going on. One small change I'd like to see as we discussed on IRC is moving the ownership of the WebLayer exclusively to the WebMediaPlayer and exposing only a non-owning pointer to WebMediaPlayerClientImpl.  It looks like the way that would be most consistent with how plugins work would be to add

  virtual void setWebLayer(WebLayer*)

to WebMediaPlayerClient and in the implementation figure out whether that means to register/unregister the layer and then store a (raw, weak) pointer in a member on WebMediaPlayerClientImpl.

(r- for now to get out of the queue, but otherwise this looks super nice)
Comment 28 Dana Jansens 2013-03-20 15:42:24 PDT
Created attachment 194137 [details]
Patch

moved ownership
Comment 29 WebKit Review Bot 2013-03-20 15:55:17 PDT
Comment on attachment 194137 [details]
Patch

Attachment 194137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17246139
Comment 30 Peter Beverloo (cr-android ews) 2013-03-20 16:08:21 PDT
Comment on attachment 194137 [details]
Patch

Attachment 194137 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17224207
Comment 31 WebKit Review Bot 2013-03-20 17:14:54 PDT
Comment on attachment 194137 [details]
Patch

Attachment 194137 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17145760
Comment 32 James Robinson 2013-03-20 18:11:07 PDT
Comment on attachment 194137 [details]
Patch

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

This looks great.  How do we stage this for landing?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:283
> +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer)

I think you need to kick a synthetic style recalc too so the video layer gets added to the compositing tree.  See WebPluginContainerImpl::setWebLayer().  is this happening somewhere else currently?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:289
> +        m_videoLayer->setOpaque(m_opaque);

I think this would be better set on the chromium side of the world - in fact m_opaque doesn't need to exist in the WebKit side at all.  but you don't have to do that in the first patch.
Comment 33 Dana Jansens 2013-03-20 20:22:32 PDT
Comment on attachment 194137 [details]
Patch

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

I'm thinking that tomorrow morning I will basically put #ifndef NEW_MEDIA_OWNERSHIP around all the - lines in the chromium diff, and #ifdef NEW_MEDIA_OWNERSHIP around all the + lines in the chromium diff, and go for landing that first.

Then, once it's landed, stick a #define NEW_MEDIA_OWNERSHIP in WebMediaPlayer.h or something, and land this patch as is.

Finally, after a webkit roll, go back and remove all the #ifndefs, as well as the #ifdefs and all unused code inside them.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:283
>> +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer)
> 
> I think you need to kick a synthetic style recalc too so the video layer gets added to the compositing tree.  See WebPluginContainerImpl::setWebLayer().  is this happening somewhere else currently?

I don't think anything has been doing that before, and I believe it's because we just don't make a webLayer if we're not going to use compositing (see supportsAcceleratedRendering()).

Going a step at a time I've cleaned up the whole "are we going to composite this?" mess, now it looks like this:

WMPI asks WMPCI if needsWebLayer() which is true if we're using compositing.
WMPI only makes a WebLayer if ^ is true, and calls setWebLayer() with it.
WMPCI only reports supportsAcceleratedRendering() if it has a WebLayer.


Previously, WMPI would change supportsAcceleratedRendering()'s value from true to false if the content has no video in it. Now it just doesn't make a weblayer, so you get the same result. There is a subtle change, but I tested things and they seem to be working - both with and without --disable-accelerated-video. The change is that before getting the video metadata, this function would have reported "true" before even though it did not know yet. Now it reports false. But this seems to do the right thing, as RLCompositor gets the right answer once we have a video layer.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:289
>> +        m_videoLayer->setOpaque(m_opaque);
> 
> I think this would be better set on the chromium side of the world - in fact m_opaque doesn't need to exist in the WebKit side at all.  but you don't have to do that in the first patch.

Agree. It should be easy after this.
Comment 34 Dana Jansens 2013-03-20 20:27:16 PDT
Created attachment 194171 [details]
Patch

Cleaned up decision to create weblayer with ownership of the layer in WMPI
Comment 35 Peter Beverloo (cr-android ews) 2013-03-20 21:27:15 PDT
Comment on attachment 194171 [details]
Patch

Attachment 194171 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17290034
Comment 36 WebKit Review Bot 2013-03-20 23:12:19 PDT
Comment on attachment 194171 [details]
Patch

Attachment 194171 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17251106
Comment 37 WebKit Review Bot 2013-03-21 01:56:04 PDT
Comment on attachment 194171 [details]
Patch

Attachment 194171 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17255013
Comment 38 Dana Jansens 2013-03-21 15:05:26 PDT
Review for landing moving to https://bugs.webkit.org/show_bug.cgi?id=112971. This CL will be a followup to remove the then-unneeded WebVideo* classes.
Comment 39 Dana Jansens 2013-03-25 17:31:18 PDT
Created attachment 194962 [details]
Patch
Comment 40 Dana Jansens 2013-03-25 17:32:13 PDT
Created attachment 194963 [details]
Patch
Comment 41 Dana Jansens 2013-03-25 17:32:52 PDT
Latest patch just removes code that won't be used once https://codereview.chromium.org/12902002/ lands/rolls.
Comment 42 Dana Jansens 2013-03-26 17:23:08 PDT
Created attachment 195195 [details]
Patch

Chromium side has rolled into webkit. Updating for EWS and review
Comment 43 James Robinson 2013-03-26 17:32:47 PDT
Comment on attachment 195195 [details]
Patch

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

R+ for awesome, CQ- for doubled changelogs

> Source/Platform/ChangeLog:18
> +2013-03-21  Dana Jansens  <danakj@chromium.org>

double changelog all the way

> Source/WebKit/chromium/ChangeLog:17
> +2013-03-21  Dana Jansens  <danakj@chromium.org>

ditto here
Comment 44 Dana Jansens 2013-03-26 17:36:58 PDT
Created attachment 195199 [details]
Patch

Single changelogs!
Comment 45 WebKit Review Bot 2013-03-26 18:23:34 PDT
Comment on attachment 195199 [details]
Patch

Clearing flags on attachment: 195199

Committed r146957: <http://trac.webkit.org/changeset/146957>