Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10914237:
Port perf.py's _LoginToGoogleAccount to chrome_remote_control (Closed)
|
Created:
8 years, 3 months ago by hartmanng Modified:
8 years, 2 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPort perf.py's _LoginToGoogleAccount to chrome_remote_control
Added LoginToGoogleAccount functionality into chrome_remote_control, letting us
use it in run_scrolling_benchmark.
BUG=147612
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159858
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 25
Patch Set 3 : fixes and unit tests #
Total comments: 4
Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 33 (0 generated)
Please take a look. Note that currently, if you want to log into a Google account for a test, you have to add the account credentials to perf.cfg before running, and you also have to specify the --login-to-google-account command-line flag.
https://chromiumcodereview.appspot.com/10914237/diff/1/tools/chrome_remote_co... File tools/chrome_remote_control/chrome_remote_control/desktop_browser_backend.py (right): https://chromiumcodereview.appspot.com/10914237/diff/1/tools/chrome_remote_co... tools/chrome_remote_control/chrome_remote_control/desktop_browser_backend.py:63: """Return eval of python code from given file. Lets get @dtu's page-specific features patch in. He's working on it. Then, 'needs google login' should be an attribute on a page. The actual LoginToGoogle should be a method on a standalone file called tab_google_account.py. tab.py should have a member variable called google_account that is a TabGoogleAccount. Then, the page.SetUp should call tab.google_account.LoginNeeded(). page.TearDown() should casll tab.google_Account.LoginNoLongerNeeded(). That NoLongerNeeded can be a noop for now, but it will enforce a "Alloc/Free" pattern onto the design that will be healthy for testability. Make sense? Feel free to reach out if I make absolutely no sense. :) This "tearoff" functionality is just to keep features for tabs from bloating tab.py and facilitate unit testing.
Also whats this perf.cfg thing?
On 2012/09/14 17:18:36, nduca wrote: > https://chromiumcodereview.appspot.com/10914237/diff/1/tools/chrome_remote_co... > File > tools/chrome_remote_control/chrome_remote_control/desktop_browser_backend.py > (right): > > https://chromiumcodereview.appspot.com/10914237/diff/1/tools/chrome_remote_co... > tools/chrome_remote_control/chrome_remote_control/desktop_browser_backend.py:63: > """Return eval of python code from given file. > Lets get @dtu's page-specific features patch in. He's working on it. Then, > 'needs google login' should be an attribute on a page. > > The actual LoginToGoogle should be a method on a standalone file called > tab_google_account.py. tab.py should have a member variable called > google_account that is a TabGoogleAccount. Then, the page.SetUp should call > tab.google_account.LoginNeeded(). page.TearDown() should casll > tab.google_Account.LoginNoLongerNeeded(). That NoLongerNeeded can be a noop for > now, but it will enforce a "Alloc/Free" pattern onto the design that will be > healthy for testability. > > Make sense? Feel free to reach out if I make absolutely no sense. :) > > This "tearoff" functionality is just to keep features for tabs from bloating > tab.py and facilitate unit testing. I think all this makes sense, but I feel like it could result in logging in to a Google account many times during a test, instead of just once when we start the browser (if I'm interpreting this properly). Is this what we want? Also, when you talk about "page" I assume you mean .../chrome_remote_control/tab_page.py? It doesn't have SetUp or TearDown functions at the moment, is that something @dtu is adding? On 2012/09/14 17:19:03, nduca wrote: > Also whats this perf.cfg thing? That's how pyauto defined login credentials for the Google account. I guess it should be renamed for this context, but I liked the idea of having a cfg file for the login/password instead of specifying them as command-line arguments. Seems less... transparent?
> I think all this makes sense, but I feel like it could result in logging in to a > Google account many times during a test, instead of just once when we start the > browser (if I'm interpreting this properly). Is this what we want? Make the logout a nop, and remember if you're logged in. Et voila, done. > Also, when you talk about "page" I assume you mean > .../chrome_remote_control/tab_page.py? It doesn't have SetUp or TearDown > functions at the moment, is that something @dtu is adding? page_set.py's Page class is migtrating out to page.py --- dtu is doing or I'll do this weekend. Its very high priority. > On 2012/09/14 17:19:03, nduca wrote: > > Also whats this perf.cfg thing? > > That's how pyauto defined login credentials for the Google account. I guess it > should be renamed for this context, but I liked the idea of having a cfg file > for the login/password instead of specifying them as command-line arguments. > Seems less... transparent? I guess I dont understand what the syntax is... its like shell script or something?
Oh i see, perf.cfg is some json object. We should do this right: - the suffix should be json - the file should be somewhere else, maybe inside page_sets/ - credentials.json seems like a better title than perf.cfg - we should aspire to make the config file generalized so that facebook login can be added too - which implies tab_credentials.py, which calls out to google_credentials_backend.py for login services. We can discuss in a hangout if you want? Point is, come up with a class design that makes sense that isn't "google specific" (within reason)
On 2012/09/15 00:26:42, nduca wrote: > Oh i see, perf.cfg is some json object. We should do this right: > - the suffix should be json > - the file should be somewhere else, maybe inside page_sets/ > - credentials.json seems like a better title than perf.cfg It's actually interpreted as python (via eval()), effectively it is translated directly into a python dictionary. I can put it in page_sets/credentials.py if you want? > - we should aspire to make the config file generalized so that facebook login > can be added too > - which implies tab_credentials.py, which calls out to > google_credentials_backend.py for login services. We can discuss in a hangout if > you want? Point is, come up with a class design that makes sense that isn't > "google specific" (within reason) I might want to clarify this part over a hangout, maybe Monday?
On 2012/09/15 00:32:47, hartmanng wrote: > It's actually interpreted as python (via eval()), effectively it is translated > directly into a python dictionary. I can put it in page_sets/credentials.py if > you want? Oh, right. Sorry about taht. This having been said, the pagesets are json, so it seems we should be self-consistent with ourselves. Wdyt? > I might want to clarify this part over a hangout, maybe Monday? Sure thing. :)
On 2012/09/15 00:56:03, nduca wrote: > On 2012/09/15 00:32:47, hartmanng wrote: > > > It's actually interpreted as python (via eval()), effectively it is translated > > directly into a python dictionary. I can put it in page_sets/credentials.py if > > you want? > > Oh, right. Sorry about taht. > > This having been said, the pagesets are json, so it seems we should be > self-consistent with ourselves. Wdyt? Good point, I'll convert the credentials file into JSON too then.
Hey glenn: @dtu's page_runner patch has landed so we should be able to resume work on this.
Hey Nat, I think this might be more what you were looking for. Please take another look.
This looks really nice. - credentials are shared across tabs. So, the state for credentials needs to get stashed on the browser object... but you need a tab in order to login, so maybe the thing to do is make the Login/Unlogin calls be just on the tab, but then have the state for the overall credentials object live off of browser, just browser_credentials.py ... or as straight-up the backend code can stay the same, maybe? We can chat on gchat if this its unclear how to proceed. note you can get the browser from a tab by saying "tab.browser". You'll need unit tests. :)
http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:23: contents = f.read() extra space before = http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:46: js = "document.getElementById(\"%s\").submit();" % form_id use single tics for consistency and then you can drop the escaping around the %s http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py:43: time.sleep(2) # Wait for unpredictable redirects. Looks brittle. Is there anything better to WaitFor rather than sleeping? http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/tab_credentials.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/tab_credentials.py:18: def LoginNeeded(self, credentials_type): Wonder if LoginNeeded/LoginNoLongerNeeded should be __enter__/__exit__ http://codereview.chromium.org/10914237/diff/9001/tools/gpu/page_sets/credent... File tools/gpu/page_sets/credentials.json (right): http://codereview.chromium.org/10914237/diff/9001/tools/gpu/page_sets/credent... tools/gpu/page_sets/credentials.json:3: "google_password": null, are we really going to check the password into the public repo? Seems more appropriate for src-internal.
http://codereview.chromium.org/10914237/diff/9001/tools/gpu/page_sets/credent... File tools/gpu/page_sets/credentials.json (right): http://codereview.chromium.org/10914237/diff/9001/tools/gpu/page_sets/credent... tools/gpu/page_sets/credentials.json:3: "google_password": null, I think this is just an example file to document how this works with the intent that we have a real file in src-internal that we pull in. Maybe we should add some notes here. Maybe we should call this empty_credentials.json and a companion file, credentials_format.txt explaining the format?
https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:14: pass raise NotImplementedError() is the Pythonic way of defining an abstract method. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:17: pass raise NotImplementedError() https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py:23: def LoginNoLongerNeeded(self): This is a placeholder for the future, right? What are some potential scenarios in which this method will be needed/used? https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/page_runner.py:33: if hasattr(page, 'credentials'): if page.credentials: credentials is defined as None in Page's constructor, so hasattr isn't right. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/tab_credentials.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/tab_credentials.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. I kind of think this class is unnecessary and the credentials_backend should be owned by the PageRunner, not the tab. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/gpu/page_sets... File tools/gpu/page_sets/credentials.json (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/gpu/page_sets... tools/gpu/page_sets/credentials.json:3: "google_password": null, On 2012/09/24 20:50:00, nduca wrote: > I think this is just an example file to document how this works with the intent > that we have a real file in src-internal that we pull in. Maybe we should add > some notes here. Maybe we should call this empty_credentials.json and a > companion file, credentials_format.txt explaining the format? I think the script should check this file first and fall back to the internal file. Then public contributors can put their own credentials in this file to run the test. So empty_credentials.json is not a good name, but a comment would be useful telling people how to use this file.
https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/tab_credentials.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/tab_credentials.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/24 21:35:10, Dave Tu wrote: > I kind of think this class is unnecessary and the credentials_backend should be > owned by the PageRunner, not the tab. Saw nduca's comment. His way works better, so the browser can keep track of the credentials state.
http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:10: _config_file = 'page_sets/credentials.json' This makes an assumption about where the script is run from. I was just experimenting with the patch and was attempting to run the benchmarks from the src/ directory and had no luck.
http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:10: _config_file = 'page_sets/credentials.json' ah good point. The right way to write this is: os.path.join(os.dirname(__file__), "..", "page_sets", "credentials.json") In the future, maybe not this patch, maybe credentials.json should found relative to the current pageset?
Sorry for the spam. Noticed something else. http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... tools/chrome_remote_control/chrome_remote_control/page_runner.py:45: tab.credentials.LoginNoLongerNeeded(page.credentials) This causes exceptions. Will need to pass in page or store it in PreparePage (I would pass it in for symmetry and because it's nice to have less state in the runner).
On 2012/09/25 03:00:18, vollick wrote: > Sorry for the spam. Noticed something else. > > http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... > File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): > > http://codereview.chromium.org/10914237/diff/9001/tools/chrome_remote_control... > tools/chrome_remote_control/chrome_remote_control/page_runner.py:45: > tab.credentials.LoginNoLongerNeeded(page.credentials) > This causes exceptions. Will need to pass in page or store it in PreparePage (I > would pass it in for symmetry and because it's nice to have less state in the > runner). Whoops, forgot to mention that tab will also be needed.
I think I've addressed all the issues, if I missed something please let me know. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:10: _config_file = 'page_sets/credentials.json' On 2012/09/25 02:00:41, vollick wrote: > This makes an assumption about where the script is run from. I was just > experimenting with the patch and was attempting to run the benchmarks from the > src/ directory and had no luck. Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:14: pass On 2012/09/24 21:35:10, Dave Tu wrote: > raise NotImplementedError() is the Pythonic way of defining an abstract method. Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:17: pass On 2012/09/24 21:35:10, Dave Tu wrote: > raise NotImplementedError() Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:23: contents = f.read() On 2012/09/24 20:41:01, tonyg wrote: > extra space before = Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:46: js = "document.getElementById(\"%s\").submit();" % form_id On 2012/09/24 20:41:01, tonyg wrote: > use single tics for consistency and then you can drop the escaping around the %s Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py:23: def LoginNoLongerNeeded(self): On 2012/09/24 21:35:10, Dave Tu wrote: > This is a placeholder for the future, right? What are some potential scenarios > in which this method will be needed/used? I added this in response to http://codereview.chromium.org/10914237/#msg2 - it's my understanding that we may just leave this as a no-op. Probably best if @nduca answers this though. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/google_credentials_backend.py:43: time.sleep(2) # Wait for unpredictable redirects. On 2012/09/24 20:41:01, tonyg wrote: > Looks brittle. Is there anything better to WaitFor rather than sleeping? Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/page_runner.py:33: if hasattr(page, 'credentials'): On 2012/09/24 21:35:10, Dave Tu wrote: > if page.credentials: > > credentials is defined as None in Page's constructor, so hasattr isn't right. Done. https://chromiumcodereview.appspot.com/10914237/diff/9001/tools/chrome_remote... tools/chrome_remote_control/chrome_remote_control/page_runner.py:45: tab.credentials.LoginNoLongerNeeded(page.credentials) On 2012/09/25 03:00:18, vollick wrote: > This causes exceptions. Will need to pass in page or store it in PreparePage (I > would pass it in for symmetry and because it's nice to have less state in the > runner). Done.
Getting there! I'm very sorry for the delayed review... http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:12: 'page_sets', 'credentials.json') So this bit doesn't work... what if we have multiple tools using chrome-remote-control? We probably need to have the credentials file be an attribute on the page_set json.Copy tonyg's change in http://codereview.chromium.org/10965027/ if its not clear... When the page set starts, you can say browser.SetCredentialsConfigFile(blahblah). http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/credentials_backend_unittest.py (right): http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/credentials_backend_unittest.py:39: def setup(): Maybe getPossibleBrowser
PTAL In addition to your comments, I also fixed a bunch of pylint issues (which unfortunately included taking out one or two unit tests since it didn't like me directly calling protected functions). I also switched some of credentials_backend to use @classmethod where appropriate instead of @staticmethod. The credentials_path in the page_sets are relative to the page_set. http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/credentials_backend.py (right): http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/credentials_backend.py:12: 'page_sets', 'credentials.json') On 2012/10/02 03:49:12, nduca wrote: > So this bit doesn't work... what if we have multiple tools using > chrome-remote-control? We probably need to have the credentials file be an > attribute on the page_set json.Copy tonyg's change in > http://codereview.chromium.org/10965027/ if its not clear... > > When the page set starts, you can say > browser.SetCredentialsConfigFile(blahblah). Done. http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/credentials_backend_unittest.py (right): http://codereview.chromium.org/10914237/diff/18010/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/credentials_backend_unittest.py:39: def setup(): On 2012/10/02 03:49:12, nduca wrote: > Maybe getPossibleBrowser Done.
http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_runner.py:30: credentials_backend.CredentialsBackend.SetCredentialsConfigFile( Why are you making this a static method? This should really be part of the browser-level value... by making it a global, you're preventing us from having two browser instances with different credentials paths. http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_runner.py:31: self.page_set.credentials_path) what if the credentials_path is none? http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_set.py (right): http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:29: credentials_path='credentials.json'): can we just default to None, and assume no credentials if credentials_path=None? http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:43: def GetCredentialsPath(data, file_path): this bit i dont quite get... why are you computing this here rather than in the constructor? http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:54: page_set = cls(data['description'], file_path, credentials_path) if you set credentials path to be the fully qualified location here, then you can avoid this mess. http://codereview.chromium.org/10914237/diff/30001/tools/gpu/page_sets/top_25... File tools/gpu/page_sets/top_25.json (left): http://codereview.chromium.org/10914237/diff/30001/tools/gpu/page_sets/top_25... tools/gpu/page_sets/top_25.json:3: "pages": [ isn't there another json in this folder? q3somethingorother? or does that not point at gmail?
PTAL http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right): http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_runner.py:30: credentials_backend.CredentialsBackend.SetCredentialsConfigFile( On 2012/10/02 18:02:10, nduca wrote: > Why are you making this a static method? This should really be part of the > browser-level value... by making it a global, you're preventing us from having > two browser instances with different credentials paths. Done. http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_runner.py:31: self.page_set.credentials_path) On 2012/10/02 18:02:10, nduca wrote: > what if the credentials_path is none? Then we're assuming that no credentials are needed right? (As in http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... )? If you're asking if this will crash, it won't unless someone tries to pull in credentials without specifying a file. The credentials_path member variable isn't used unless a page requests credentials. http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_set.py (right): http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:29: credentials_path='credentials.json'): On 2012/10/02 18:02:10, nduca wrote: > can we just default to None, and assume no credentials if credentials_path=None? Done. http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:43: def GetCredentialsPath(data, file_path): On 2012/10/02 18:02:10, nduca wrote: > this bit i dont quite get... why are you computing this here rather than in the > constructor? Done. http://codereview.chromium.org/10914237/diff/30001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:54: page_set = cls(data['description'], file_path, credentials_path) On 2012/10/02 18:02:10, nduca wrote: > if you set credentials path to be the fully qualified location here, then you > can avoid this mess. Done. http://codereview.chromium.org/10914237/diff/30001/tools/gpu/page_sets/top_25... File tools/gpu/page_sets/top_25.json (left): http://codereview.chromium.org/10914237/diff/30001/tools/gpu/page_sets/top_25... tools/gpu/page_sets/top_25.json:3: "pages": [ On 2012/10/02 18:02:10, nduca wrote: > isn't there another json in this folder? q3somethingorother? or does that not > point at gmail? yeah, it doesn't look like page_sets/Q32012.json needs credentials right now.
http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/browser.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/browser.py:50: def LoginNeeded(self, credentials_type, tab): Not sure you really need these three functions. Credentials is public, so just have them call that directly, no? browser.credentials.LoginNeeded http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/browser_credentials.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/browser_credentials.py:36: self.credentials_config.config_file = credentials_path how does this even work? You've read the config file up in __init__ so... http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_set.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:28: def __init__(self, description='', file_path='', lets make this take only an attributes=None array and do the same init flow as the page constructor does. http://codereview.chromium.org/10914237/diff/39001/tools/gpu/page_sets/creden... File tools/gpu/page_sets/credentials.json (right): http://codereview.chromium.org/10914237/diff/39001/tools/gpu/page_sets/creden... tools/gpu/page_sets/credentials.json:1: { please point the other pageset at the credentials file even though it doesn't need it. I have my reasons. :)
http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/browser.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/browser.py:50: def LoginNeeded(self, credentials_type, tab): On 2012/10/02 20:41:49, nduca wrote: > Not sure you really need these three functions. Credentials is public, so just > have them call that directly, no? browser.credentials.LoginNeeded Done. http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/browser_credentials.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/browser_credentials.py:36: self.credentials_config.config_file = credentials_path On 2012/10/02 20:41:49, nduca wrote: > how does this even work? You've read the config file up in __init__ so... Done. http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... File tools/chrome_remote_control/chrome_remote_control/page_set.py (right): http://codereview.chromium.org/10914237/diff/39001/tools/chrome_remote_contro... tools/chrome_remote_control/chrome_remote_control/page_set.py:28: def __init__(self, description='', file_path='', On 2012/10/02 20:41:49, nduca wrote: > lets make this take only an attributes=None array and do the same init flow as > the page constructor does. Done. http://codereview.chromium.org/10914237/diff/39001/tools/gpu/page_sets/creden... File tools/gpu/page_sets/credentials.json (right): http://codereview.chromium.org/10914237/diff/39001/tools/gpu/page_sets/creden... tools/gpu/page_sets/credentials.json:1: { On 2012/10/02 20:41:49, nduca wrote: > please point the other pageset at the credentials file even though it doesn't > need it. I have my reasons. :) Done.
LGTM please land ASAP so I can make some changes to this. I'm not quite loving how this is set up, but I think given we work in remote sites, its going to be easier for me to make the changes than keep trying to have you guess at whats in my head. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/10914237/44006
Presubmit check for 10914237-44006 failed and returned exit status 1. ************* Module chrome_remote_control.credentials_backend_unittest E1101: 51,4:TestCredentialsBackend.testSubmitForm: Instance of 'TestCredentialsBackend' has no 'assertIsNotNone' member E1101: 57,6:TestCredentialsBackend.testSubmitForm: Instance of 'TestCredentialsBackend' has no 'skipTest' member E1101: 83,4:TestCredentialsBackend.testWaitForFormToLoad: Instance of 'TestCredentialsBackend' has no 'assertIsNotNone' member E1101: 96,4:TestCredentialsBackend.testSubmitFormAndWait: Instance of 'TestCredentialsBackend' has no 'assertIsNotNone' member E1101:102,6:TestCredentialsBackend.testSubmitFormAndWait: Instance of 'TestCredentialsBackend' has no 'skipTest' member ************* Module chrome_remote_control.google_credentials_backend_unittest E1101: 15,4:TestGoogleCredentialsBackend.testGoogleAccountsLogin: Instance of 'TestGoogleCredentialsBackend' has no 'assertIsNotNone' member E1101: 23,6:TestGoogleCredentialsBackend.testGoogleAccountsLogin: Instance of 'TestGoogleCredentialsBackend' has no 'skipTest' member Running presubmit commit checks ... ** Presubmit ERRORS ** Fix pylint errors first. Presubmit checks took 6.5s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/10914237/52021
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/10914237/52021
Change committed as 159858 |
