Issue 8506034: Implement some functions in testing_automation_provider_aura.cc

Issue 8506034: Implement some functions in testing_automation_provider_aura.cc (Closed)

Created:
9 years, 1 month ago by benrg

Modified:
9 years, 1 month ago

Reviewers:
sky, Emmanuel Saint-loubert-Bié

CC:
chromium-reviews, robertshield, kkania, Paweł Hajdan Jr., Emmanuel Saint-loubert-Bié

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Implement some functions in testing_automation_provider_aura.cc BUG=99706 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110007

Patch Set 2 : fix all comments #

Total comments: 2

Patch Set 3 : fit nit and fix build (oops) #

Created: 9 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+47 lines, -13 lines)			Patch
	M	chrome/browser/automation/testing_automation_provider_aura.cc	View	1 2	1 chunk	+47 lines, -13 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Emmanuel Saint-loubert-Bié

Hi Scott, Could you please review this CL from BenRG? I do not know the ...

9 years, 1 month ago (2011-11-14 17:26:47 UTC) #1

sky

http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc File chrome/browser/automation/testing_automation_provider_aura.cc (left): http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#oldcode5 chrome/browser/automation/testing_automation_provider_aura.cc:5: #include "chrome/browser/automation/testing_automation_provider.h" For .cc files you want this format ...

9 years, 1 month ago (2011-11-14 17:39:24 UTC) #2

benrg

PTAL http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc File chrome/browser/automation/testing_automation_provider_aura.cc (left): http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#oldcode5 chrome/browser/automation/testing_automation_provider_aura.cc:5: #include "chrome/browser/automation/testing_automation_provider.h" On 2011/11/14 17:39:24, sky wrote: > ...

9 years, 1 month ago (2011-11-14 20:50:53 UTC) #3

Emmanuel Saint-loubert-Bié

Actually since there are 2 reviewers listed, this in not for me to review: Sky ...

9 years, 1 month ago (2011-11-14 21:07:11 UTC) #4

Actually since there are 2 reviewers listed, this in not for me to review:
Sky can you PTAL?

Thanks,

-- E

On Mon, Nov 14, 2011 at 12:50 PM, <benrg@chromium.org> wrote:

> Reviewers: sky, Emmanuel Saint-loubert,
>
> Message:
> PTAL
>
>
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc>
> File chrome/browser/automation/**testing_automation_provider_**aura.cc
> (left):
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc#**oldcode5<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#oldcode5>
> chrome/browser/automation/**testing_automation_provider_**aura.cc:5:
> #include "chrome/browser/automation/**testing_automation_provider.h"
> On 2011/11/14 17:39:24, sky wrote:
>
>> For .cc files you want this format for includes: include header of
>>
> file you're
>
>> implementing first, then newline, then remaining includes. See
>> testing_automation_provider_**win.cc for an example.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc>
> File chrome/browser/automation/**testing_automation_provider_**aura.cc
> (right):
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc#**newcode22<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#newcode22>
> chrome/browser/automation/**testing_automation_provider_**aura.cc:22: //
> TODO(benrg): how?
> On 2011/11/14 17:39:24, sky wrote:
>
>> This state is stored in the properties of the window unrder the key
>> aura::kShowStateKey.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc#**newcode28<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#newcode28>
> chrome/browser/automation/**testing_automation_provider_**aura.cc:28: //
> currently unimplemented in most other providers.
> On 2011/11/14 17:39:24, sky wrote:
>
>> Set *success to false here.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8506034/diff/1/chrome/**
>
browser/automation/testing_**automation_provider_aura.cc#**newcode71<http://codereview.chromium.org/8506034/diff/1/chrome/browser/automation/testing_automation_provider_aura.cc#newcode71>
> chrome/browser/automation/**testing_automation_provider_**aura.cc:71: //
> TODO(benrg): Windows provider uses this pointer without testing for
> NULL.
> On 2011/11/14 17:39:24, sky wrote:
>
>> You should NULL check.
>>
>
> Done.
>
> Description:
> Implement some functions in testing_automation_provider_**aura.cc
>
>
> BUG=99706
> TEST=none
>
>
> Please review this at
http://codereview.chromium.**org/8506034/<http://codereview.chromium.org/8506...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/automation/**testing_automation_provider_**aura.cc
>
>
> Index: chrome/browser/automation/**testing_automation_provider_**aura.cc
> diff --git a/chrome/browser/automation/**testing_automation_provider_**aura.cc
> b/chrome/browser/automation/**testing_automation_provider_**aura.cc
> index 191dbbbb1da905ed855d70f08224de**5f199bd2d6..**
> 88130af6dfc2b6abacdc99be1c00cb**be4fe50715 100644
> --- a/chrome/browser/automation/**testing_automation_provider_**aura.cc
> +++ b/chrome/browser/automation/**testing_automation_provider_**aura.cc
> @@ -5,47 +5,76 @@
>  #include "chrome/browser/automation/**testing_automation_provider.h"
>
>  #include "base/logging.h"
> +#include "chrome/browser/automation/**automation_window_tracker.h"
> +#include "ui/aura/window.h"
>
>  void TestingAutomationProvider::**ActivateWindow(int handle) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  if (aura::Window* window = window_tracker_->GetResource(**handle)) {
> +    window->Activate();
> +  } else {
> +    // TODO(benrg): Is it correct to fail silently? The Windows provider
> does.
> +  }
>  }
>
>  void TestingAutomationProvider::**IsWindowMaximized(int handle,
>                                                   bool* is_maximized,
>                                                   bool* success) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  if (aura::Window* window = window_tracker_->GetResource(**handle)) {
> +    int show_state = window->GetIntProperty(aura::**kShowStateKey);
> +    *is_maximized = (show_state == SHOW_STATE_MAXIMIZED);
> +    *success = true;
> +  } else {
> +    *success = false;
> +  }
>  }
>
>  void TestingAutomationProvider::**TerminateSession(int handle, bool*
> success) {
> -  // TODO(beng):
> +  // TODO(benrg): what should this do in aura? It's
>
> +  // currently unimplemented in most other providers.
> +  *success = false;
>   NOTIMPLEMENTED();
>  }
>
>  void TestingAutomationProvider::**GetWindowBounds(int handle,
>                                                 gfx::Rect* bounds,
>                                                 bool* success) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  if (const aura::Window* window = window_tracker_->GetResource(**handle))
> {
> +    *bounds = window->bounds();
> +    *success = true;
> +  } else {
> +    *success = false;
> +  }
>  }
>
>  void TestingAutomationProvider::**SetWindowBounds(int handle,
>                                                 const gfx::Rect& bounds,
>                                                 bool* success) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  if (aura::Window* window = window_tracker_->GetResource(**handle)) {
> +    window->SetBounds(bounds);
> +    *success = true;
> +  } else {
> +    *success = false;
> +  }
>  }
>
>  void TestingAutomationProvider::**SetWindowVisible(int handle,
>                                                  bool visible,
>                                                  bool* result) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  if (aura::Window* window = window_tracker_->GetResource(**handle)) {
> +    if (visible) {
> +      window->Show();
> +    } else {
> +      window->Hide();
> +    }
> +    *result = true;
> +  } else {
> +    *result = false;
> +  }
>  }
>
>  void TestingAutomationProvider::**GetWindowTitle(int handle, string16*
> text) {
> -  // TODO(beng):
> -  NOTIMPLEMENTED();
> +  const aura::Window* window = window_tracker_->GetResource(**handle);
> +  DCHECK(window);
> +  *text = window->title();
>  }
>
>
>
>

sky

http://codereview.chromium.org/8506034/diff/4001/chrome/browser/automation/testing_automation_provider_aura.cc File chrome/browser/automation/testing_automation_provider_aura.cc (right): http://codereview.chromium.org/8506034/diff/4001/chrome/browser/automation/testing_automation_provider_aura.cc#newcode22 chrome/browser/automation/testing_automation_provider_aura.cc:22: if (aura::Window* window = window_tracker_->GetResource(handle)) { nit: move assignment ...

9 years, 1 month ago (2011-11-14 21:25:10 UTC) #5

benrg

9 years, 1 month ago (2011-11-14 22:59:40 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/8506034/7001

9 years, 1 month ago (2011-11-15 00:11:17 UTC) #10

Change committed as 110007

Expand Messages | Collapse Messages