Definition for new API method in WebKit::WebFrame.

Definition for new API method in WebKit::WebFrame.

The new method (pageDismissalEventBeingDispatched) has not yet been added to
the API (see https://bugs.webkit.org/show_bug.cgi?id=55844). When it is, it
will be a pure virtual method; clients will break unless they already have a
concrete implementation for it.

BUG=68780
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77101

[sreeram, 2011-03-07 03:07:23]

Please review.

[Nico, 2011-03-07 03:16:35]

LG

On Sun, Mar 6, 2011 at 7:07 PM,  <sreeram@chromium.org> wrote:
> Reviewers: darin, tonyg, Nico,
>
> Message:
> Please review.
>
> Description:
> Definition for new API method in WebKit::WebFrame.
>
> The new method (pageDismissalEventBeingDispatched) has not yet been added to
> the API (see https://bugs.webkit.org/show_bug.cgi?id=55844). When it is, it
> will be a pure virtual method; clients will break unless they already have a
> concrete implementation for it.
>
> BUG=68780
> TEST=none
>
> Please review this at http://codereview.chromium.org/6626047/
>
> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>
> Affected files:
>  M webkit/mocks/mock_webframe.h
>  M webkit/mocks/mock_webframe.cc
>
>
> Index: webkit/mocks/mock_webframe.cc
> diff --git a/webkit/mocks/mock_webframe.cc b/webkit/mocks/mock_webframe.cc
> index
>
a230cc0264883b324526cf03ddb6f9af04683881..1423039813dd11faddb749b2c858da3d1b01f17b
> 100644
> --- a/webkit/mocks/mock_webframe.cc
> +++ b/webkit/mocks/mock_webframe.cc
> @@ -235,6 +235,10 @@ bool MockWebFrame::willSuppressOpenerInNewFrame() const
> {
>   return false;
>  }
>
> +bool MockWebFrame::pageDismissalEventBeingDispatched() const {
> +  return false;
> +}
> +
>  void MockWebFrame::replaceSelection(const WebString& text) {}
>
>  void MockWebFrame::insertText(const WebString& text) {}
> Index: webkit/mocks/mock_webframe.h
> diff --git a/webkit/mocks/mock_webframe.h b/webkit/mocks/mock_webframe.h
> index
>
830b0d63b40d3eaffb0e6e29902397ed0baf29da..adbf5158dc7039a651030cd34a4c6d41c465d22e
> 100644
> --- a/webkit/mocks/mock_webframe.h
> +++ b/webkit/mocks/mock_webframe.h
> @@ -141,6 +141,7 @@ class MockWebFrame : public WebKit::WebFrame {
>   virtual unsigned unloadListenerCount() const;
>   virtual bool isProcessingUserGesture() const;
>   virtual bool willSuppressOpenerInNewFrame() const;
> +  virtual bool pageDismissalEventBeingDispatched() const;
>   virtual void replaceSelection(const WebString& text);
>   virtual void insertText(const WebString& text);
>   virtual void setMarkedText(const WebString& text,
>
>
>

[sreeram, 2011-03-07 03:19:37]

Thanks, Nico. I don't have commit privileges, so could please you land this for
me?

[commit-bot: I haz the power, 2011-03-07 03:21:03]

No LGTM from valid reviewers yet.

[commit-bot: I haz the power, 2011-03-07 03:22:20]

No LGTM from valid reviewers yet.

[Nico, 2011-03-07 03:24:37]

LGTM

On Sun, Mar 6, 2011 at 7:19 PM,  <sreeram@chromium.org> wrote:
> Thanks, Nico. I don't have commit privileges, so could please you land this
> for
> me?
>
> http://codereview.chromium.org/6626047/
>

[Nico, 2011-03-07 03:25:11]

Maybe the commit bot will commit it…

On Sun, Mar 6, 2011 at 7:24 PM, Nico Weber <thakis@chromium.org> wrote:
> LGTM
>
> On Sun, Mar 6, 2011 at 7:19 PM,  <sreeram@chromium.org> wrote:
>> Thanks, Nico. I don't have commit privileges, so could please you land this
>> for
>> me?
>>
>> http://codereview.chromium.org/6626047/
>>
>

[Nico, 2011-03-07 03:43:28]

Got impatient waiting; landed it manually:
http://src.chromium.org/viewvc/chrome?view=rev&revision=77101

On Sun, Mar 6, 2011 at 7:25 PM, Nico Weber <thakis@chromium.org> wrote:
> Maybe the commit bot will commit it…
>
> On Sun, Mar 6, 2011 at 7:24 PM, Nico Weber <thakis@chromium.org> wrote:
>> LGTM
>>
>> On Sun, Mar 6, 2011 at 7:19 PM,  <sreeram@chromium.org> wrote:
>>> Thanks, Nico. I don't have commit privileges, so could please you land this
>>> for
>>> me?
>>>
>>> http://codereview.chromium.org/6626047/
>>>
>>
>

[dglazkov, 2011-03-07 04:06:02]

Oy! Why do we have a WebFrame subclass outside of WebKit tree?

On Sun, Mar 6, 2011 at 7:43 PM, Nico Weber <thakis@chromium.org> wrote:
> Got impatient waiting; landed it manually:
> http://src.chromium.org/viewvc/chrome?view=rev&revision=77101
>
> On Sun, Mar 6, 2011 at 7:25 PM, Nico Weber <thakis@chromium.org> wrote:
>> Maybe the commit bot will commit it…
>>
>> On Sun, Mar 6, 2011 at 7:24 PM, Nico Weber <thakis@chromium.org> wrote:
>>> LGTM
>>>
>>> On Sun, Mar 6, 2011 at 7:19 PM,  <sreeram@chromium.org> wrote:
>>>> Thanks, Nico. I don't have commit privileges, so could please you land this
>>>> for
>>>> me?
>>>>
>>>> http://codereview.chromium.org/6626047/
>>>>
>>>
>>
>

[Nico, 2011-03-07 05:35:43]

Surprisingly, it looks like this broke
nstantTest.NonSearchToSearchDoesntSupportInstant on Builder Linux Builder
(ChromiumOS): 
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS%29

ukai's reverting the change for now.

[sreeram_google.com, 2011-03-07 05:45:49]

On Sun, Mar 6, 2011 at 21:35,  <thakis@chromium.org> wrote:
> Surprisingly, it looks like this broke
> nstantTest.NonSearchToSearchDoesntSupportInstant on Builder Linux Builder
> (ChromiumOS):
>
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS%29
>
> ukai's reverting the change for now.
>
> http://codereview.chromium.org/6626047/

Could we please not revert this? Not only will this necessitate
reverting a related change in webkit
(https://bugs.webkit.org/show_bug.cgi?id=55844), but it looks like
that test failure may also be unrelated. As far as I can tell, at
least (grep for NonSearchToSearchDoesntSupportInstant at
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS...).

[sreeram_google.com, 2011-03-07 05:52:47]

On Sun, Mar 6, 2011 at 21:45, Sreeram Ramachandran <sreeram@google.com> wrote:
> On Sun, Mar 6, 2011 at 21:35,  <thakis@chromium.org> wrote:
>> Surprisingly, it looks like this broke
>> nstantTest.NonSearchToSearchDoesntSupportInstant on Builder Linux Builder
>> (ChromiumOS):
>>
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS%29
>>
>> ukai's reverting the change for now.
>>
>> http://codereview.chromium.org/6626047/
>
> Could we please not revert this? Not only will this necessitate
> reverting a related change in webkit
> (https://bugs.webkit.org/show_bug.cgi?id=55844), but it looks like
> that test failure may also be unrelated. As far as I can tell, at
> least (grep for NonSearchToSearchDoesntSupportInstant at
>
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS...).

Never mind. Looks like ukai already reverted it (r77108). Will figure
out how to test the chromium-os tests and send for review again.

[sreeram_google.com, 2011-03-07 05:58:59]

On Sun, Mar 6, 2011 at 21:51, Sreeram Ramachandran <sreeram@google.com> wrote:
> On Sun, Mar 6, 2011 at 21:45, Sreeram Ramachandran <sreeram@google.com> wrote:
>> On Sun, Mar 6, 2011 at 21:35,  <thakis@chromium.org> wrote:
>>> Surprisingly, it looks like this broke
>>> nstantTest.NonSearchToSearchDoesntSupportInstant on Builder Linux Builder
>>> (ChromiumOS):
>>>
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS%29
>>>
>>> ukai's reverting the change for now.
>>>
>>> http://codereview.chromium.org/6626047/
>>
>> Could we please not revert this? Not only will this necessitate
>> reverting a related change in webkit
>> (https://bugs.webkit.org/show_bug.cgi?id=55844), but it looks like
>> that test failure may also be unrelated. As far as I can tell, at
>> least (grep for NonSearchToSearchDoesntSupportInstant at
>>
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS...).
>
> Never mind. Looks like ukai already reverted it (r77108). Will figure
> out how to test the chromium-os tests and send for review again.

I think the revert was unnecessary. ukai's revert was r77108. However,
the failing test seems to have magically started working again in
r77106 itself:
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS...

Am I reading that right? Could we revert the revert please?