Unit tests for publishing app shortcuts on Mac

Unit tests for publishing app shortcuts on Mac

BUG=112651
TEST=Manually built a App loader bundle and copied it into the Chrome framework. Verified that tests passed.


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

[Robert Sesek, 2012-02-08 01:01:28]

I'll hold off on doing a full review until the other CL is done.

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.h:13: class WebAppShortcutCreator {
Comment++++++

[jeremy, 2012-02-08 09:14:51]

Will be easier to review this once the previous CL has landed.

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac_unittest.mm:62: [plist
objectForKey:app_mode::kCrAppModeShortcutURLKey]);
Would be good to test that the function that needs to fill in the plist is
filling in all fields.

[sail, 2012-02-09 23:04:39]

rebased, please take another look

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.h:13: class WebAppShortcutCreator {
On 2012/02/08 01:01:28, rsesek wrote:
> Comment++++++

Done.

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9362001/diff/2001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac_unittest.mm:62: [plist
objectForKey:app_mode::kCrAppModeShortcutURLKey]);
On 2012/02/08 09:14:51, jeremy wrote:
> Would be good to test that the function that needs to fill in the plist is
> filling in all fields.

I added a TODO for this. Currently there are a couple of values that we don't
fill in.

[Mihai Parparita -not on Chrome, 2012-02-11 01:54:55]

LGTM

[jeremy, 2012-02-12 12:05:46]

LGTM but I'd prefer that the plist verification happen as part of this CL, seems
like a quick test to add?

https://chromiumcodereview.appspot.com/9362001/diff/7001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9362001/diff/7001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac_unittest.mm:66: // for "@APP_*@".
Can we do this as part of this CL?

[sail, 2012-02-12 21:04:21]

https://chromiumcodereview.appspot.com/9362001/diff/7001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9362001/diff/7001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac_unittest.mm:66: // for "@APP_*@".
On 2012/02/12 12:05:46, jeremy wrote:
> Can we do this as part of this CL?

Done.

[jeremy, 2012-02-13 05:01:38]

LGTM

[Robert Sesek, 2012-02-13 21:47:19]

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.h (right):

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.h:9: #include
"chrome/browser/shell_integration.h"
IWYU: FilePath

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.mm:31:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
It occurred to me that we should be using the blocking pool (see cr-dev). Though
that's another CL for sure.

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac_unittest.mm:30:
MOCK_CONST_METHOD1(GetDstPath, FilePath(const FilePath&));
This isn't mocking the right thing. DestinationPath now.

But if this was passing, doesn't that mean the CreateFailure test doesn't do
much?

[sail, 2012-02-13 23:26:42]

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.h (right):

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.h:9: #include
"chrome/browser/shell_integration.h"
On 2012/02/13 21:47:19, rsesek wrote:
> IWYU: FilePath

Done.

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

http://codereview.chromium.org/9362001/diff/15001/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac_unittest.mm:30:
MOCK_CONST_METHOD1(GetDstPath, FilePath(const FilePath&));
On 2012/02/13 21:47:19, rsesek wrote:
> This isn't mocking the right thing. DestinationPath now.
> 
> But if this was passing, doesn't that mean the CreateFailure test doesn't do
> much?

Done.
Fixed. I didn't run the test after the last change. I'm running it now and
started a try job too.

[Robert Sesek, 2012-02-14 00:27:13]

lgtm

[commit-bot: I haz the power, 2012-02-14 00:40:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9362001/19001

[commit-bot: I haz the power, 2012-02-14 02:21:21]

Change committed as 121822