Refactor: Make PyAuto InstallExtension() take a string. Delete dead code.

Refactor: Make PyAuto InstallExtension() take a string. Delete dead code.


BUG=chromium:86890
TEST=Run the modified PyAuto tests on all platforms.


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

[Nirnimesh, 2011-08-03 01:28:18]

LGTM

http://codereview.chromium.org/7548024/diff/2001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7548024/diff/2001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1301: std::string  /* full path to
extension */,
append: crx or unbundled

[kkania, 2011-08-03 16:01:56]

http://codereview.chromium.org/7548024/diff/7001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7548024/diff/7001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:775: const FilePath
extension_file_path = FilePath(extension_path);
this wont compile on windows

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (left):

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1131: // Silently install the
extension in the given crx file.
Deprecate this instead, as seen below. Otherwise you change the line number and
break the perf tests and chromedriver.

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1301: std::string /* full path to
crx or unpacked dir */,
hmm, in chromedriver we just started depending on this message a few days ago
for installing extensions. We used this instead of creating a JSON one because
then our users would need to wait for 2 months to get the feature.

Changing this will break older chromedriver's ability to work with newer
chromes, and will break newer chromedriver's ability to work with the current
stable/beta builds until this change makes it there.

you need to modify chrome/test/webdriver/automation.cc too, which was modified a
few days ago to use this func.

I see that using a string makes it easier for python clients. However, it also
makes it slightly more difficult for c++ clients. Now we have to convert from
file paths <-> std::string.

Sorry to twist your arm a bit, but do you think you can make a new JSON call
instead of changing this? If you want, I'd be more than happy to do it myself.

[dtu, 2011-08-03 19:31:52]

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1301: std::string /* full path to
crx or unpacked dir */,
On 2011/08/03 16:01:56, kkania wrote:
> hmm, in chromedriver we just started depending on this message a few days ago
> for installing extensions. We used this instead of creating a JSON one because
> then our users would need to wait for 2 months to get the feature.
> 
> Changing this will break older chromedriver's ability to work with newer
> chromes, and will break newer chromedriver's ability to work with the current
> stable/beta builds until this change makes it there.
> 
> you need to modify chrome/test/webdriver/automation.cc too, which was modified
a
> few days ago to use this func.
> 
> I see that using a string makes it easier for python clients. However, it also
> makes it slightly more difficult for c++ clients. Now we have to convert from
> file paths <-> std::string.
> 
> Sorry to twist your arm a bit, but do you think you can make a new JSON call
> instead of changing this? If you want, I'd be more than happy to do it myself.

I can move the string -> FilePath conversion into pyautolib.cc so it doesn't
affect this signature. Is the rename of this message okay, or do you need to
maintain the original name?

[kkania, 2011-08-03 20:25:34]

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7548024/diff/7001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1301: std::string /* full path to
crx or unpacked dir */,
On 2011/08/03 19:31:53, Dave Tu wrote:
> On 2011/08/03 16:01:56, kkania wrote:
> > hmm, in chromedriver we just started depending on this message a few days
ago
> > for installing extensions. We used this instead of creating a JSON one
because
> > then our users would need to wait for 2 months to get the feature.
> > 
> > Changing this will break older chromedriver's ability to work with newer
> > chromes, and will break newer chromedriver's ability to work with the
current
> > stable/beta builds until this change makes it there.
> > 
> > you need to modify chrome/test/webdriver/automation.cc too, which was
modified
> a
> > few days ago to use this func.
> > 
> > I see that using a string makes it easier for python clients. However, it
also
> > makes it slightly more difficult for c++ clients. Now we have to convert
from
> > file paths <-> std::string.
> > 
> > Sorry to twist your arm a bit, but do you think you can make a new JSON call
> > instead of changing this? If you want, I'd be more than happy to do it
myself.
> 
> I can move the string -> FilePath conversion into pyautolib.cc so it doesn't
> affect this signature. Is the rename of this message okay, or do you need to
> maintain the original name?

Renaming is an ok approach too.

[dtu, 2011-08-03 22:40:34]

Now the conversion from string to FilePath is in pyautolib.cc, so there's no
change on the server side other than renames and delicious dead code.

[kkania, 2011-08-03 23:52:46]

LGTM, one comment

http://codereview.chromium.org/7548024/diff/17001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/7548024/diff/17001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.cc:213:
automation()->InstallExtension(FilePath(extension_file_path), with_ui);
no reason to copy the FilePath here

[commit-bot: I haz the power, 2011-08-04 21:05:28]

Change committed as 95490