Componentize sad_tab

Componentize sad_tab

Add a new component components/sad_tab.

Move sad_tab_types.h, sad_tab.{cc,h}, sad_tab_helper.{cc,h}, sad_tab_view.{cc,h} to components/sad_tab.
Move URL constants to components/sad_tab/url_constants.{cc,h}.
Move string resources to components/sad_tab_strings.grdp

Add SadTabClient and implement ChromeSadTabClient.
Fix tab_helpers.cc to pass ChromeSadTabClient to SadTabHelper.

Copy chrome/browser/ui/OWNERS to components/sad_tab.
Fix DEPS, GYP and GN.

BUG=402485
TEST=Open chrome://crash and chrome://kill

[hashimoto, 2014-09-04 10:26:38]

Patchset #4 (id:60001) has been deleted

[hashimoto, 2014-09-04 10:41:13]

Patchset #7 (id:140001) has been deleted

[hashimoto, 2014-09-04 10:41:21]

Patchset #6 (id:120001) has been deleted

[hashimoto, 2014-09-04 10:41:30]

Patchset #5 (id:100001) has been deleted

[hashimoto, 2014-09-04 10:41:37]

Patchset #4 (id:80001) has been deleted

[hashimoto, 2014-09-04 10:41:46]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-09-04 10:45:08]

Patchset #3 (id:150001) has been deleted

[hashimoto, 2014-09-04 10:46:12]

Patchset #1 (id:20001) has been deleted

[hashimoto, 2014-09-04 11:13:12]

Patchset #1 (id:40001) has been deleted

[hashimoto, 2014-09-04 11:13:28]

hashimoto@chromium.org changed reviewers:
+ pkasting@chromium.org

[hashimoto, 2014-09-04 11:13:29]

Could you review this?

[hashimoto, 2014-09-04 11:14:57]

hashimoto@chromium.org changed reviewers:
+ blundell@chromium.org

[hashimoto, 2014-09-04 11:14:58]

+blundell@ as an owner of components.

[blundell, 2014-09-05 09:08:53]

Looks good, just one main concern.

You'll have to do the internal CL adding sad_tabs.grdp.

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_sad_tab_client.cc (right):

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_sad_tab_client.cc:34: return
ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
Why does this have to go through the client, out of curiosity?

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right):

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:14: SadTab*
SadTab::Create(content::WebContents* web_contents,
Rather than having a static function declared in the component and implemented
in the embedder, I'd much rather have it be a method on the client.

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/DEPS
File components/sad_tab/DEPS (right):

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/DEPS...
components/sad_tab/DEPS:2: "+content/public",
this should be restricted to the subdirectories that are needed, e.g.
content/public/browser.

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/sad_...
File components/sad_tab/sad_tab.h (right):

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/sad_...
components/sad_tab/sad_tab.h:23: static SadTab* Create(content::WebContents*
web_contents,
as said before, could this be made a method on the client instead?

[hashimoto, 2014-09-08 10:44:40]

Patchset #2 (id:190001) has been deleted

[hashimoto, 2014-09-08 11:15:04]

Patchset #3 (id:230001) has been deleted

[hashimoto, 2014-09-08 12:02:30]

Patchset #4 (id:270001) has been deleted

[hashimoto, 2014-09-08 12:14:50]

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_sad_tab_client.cc (right):

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_sad_tab_client.cc:34: return
ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
On 2014/09/05 09:08:53, blundell wrote:
> Why does this have to go through the client, out of curiosity?

IIUC trybots were not good at handling patches with changes on binary files so I
wanted to move binary files in a separate patch to use CQ to submit the main
part of my change.
But it seems now they can handle binary files well.
Replaced this method with referring resource IDs directly.

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right):

https://codereview.chromium.org/543663002/diff/170001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:14: SadTab*
SadTab::Create(content::WebContents* web_contents,
On 2014/09/05 09:08:53, blundell wrote:
> Rather than having a static function declared in the component and implemented
> in the embedder, I'd much rather have it be a method on the client.

Done.

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/DEPS
File components/sad_tab/DEPS (right):

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/DEPS...
components/sad_tab/DEPS:2: "+content/public",
On 2014/09/05 09:08:53, blundell wrote:
> this should be restricted to the subdirectories that are needed, e.g.
> content/public/browser.

Done.

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/sad_...
File components/sad_tab/sad_tab.h (right):

https://codereview.chromium.org/543663002/diff/170001/components/sad_tab/sad_...
components/sad_tab/sad_tab.h:23: static SadTab* Create(content::WebContents*
web_contents,
On 2014/09/05 09:08:53, blundell wrote:
> as said before, could this be made a method on the client instead?

Done.

[blundell, 2014-09-08 12:22:26]

lgtm

[Peter Kasting, 2014-09-08 17:47:17]

pkasting@chromium.org changed reviewers:
+ msw@chromium.org
- pkasting@chromium.org

[Peter Kasting, 2014-09-08 17:47:18]

I don't know anything about sad_tab.  I shouldn't be in the OWNERS list for it.

I think you want msw for this change and as the contents of the OWNERS file.

[hashimoto, 2014-09-11 07:53:38]

msw@, could you take a look at this change?

On 2014/09/08 17:47:18, Peter Kasting wrote:
> I don't know anything about sad_tab.  I shouldn't be in the OWNERS list for
it.
> 
> I think you want msw for this change and as the contents of the OWNERS file.

[msw, 2014-09-15 18:14:53]

msw@chromium.org changed reviewers:
+ sadrul@chromium.org

[msw, 2014-09-15 18:14:54]

Sadrul would be a better reviewer. I agree with Peter that copying over
c/b/ui/OWNERS might not make sense.

[hashimoto, 2014-09-22 04:31:20]

sadrul@, could you take a look at this change?

[sadrul, 2014-09-22 16:48:22]

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
File components/sad_tab/sad_tab.h (right):

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
components/sad_tab/sad_tab.h:26: 
This doesn't seem to exist/be necessary in this patchset, since SadTab is
created through the SadTabClient? (see other comments about this)

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
File components/sad_tab/sad_tab_helper.cc (right):

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
components/sad_tab/sad_tab_helper.cc:57: sad_tab_ =
client_->CreateSadTab(web_contents(), kind);
My suggestion would be to: (1) move SadTabCocoa inside
//components/sad_tab/cocoa/, (2) remove Client::CreateSadTab, (3) have
SadTab::Create() create the views/cocoa version as appropriate. This way, the
app (i.e. chrome, athena) only needs to create the SadTabHelper, and provide the
implementations of IsInBrowserShutdown() and ShowFeedbackPage(), instead of also
having to know about the internal implementations of SadTabViews, or provide an
implementation of it (as this patch does with SadTabCocoa). This way, we also
don't need extra code for ChromeSadTabClient on mac.

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/url_...
File components/sad_tab/url_constants.cc (right):

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/url_...
components/sad_tab/url_constants.cc:22: 
An alternate for this is:

#if defined(OS_CHROMEOS)
#define PRODUCT_PATH "chromeos"
#else
#define PRODUCT_PATH "chrome"
#endif

const char kCrashReasonURL[] =
    "https://support.google.com/" PRODUCT_PATH "/?p=e_awsnap";
const char kKillReasonURL[] =
    "https://support.google.com/" PRODUCT_PATH "/?p=e_deadjim";

[hashimoto, 2014-09-24 09:56:25]

OK, let's move SadTabCocoa too then.
Made a patch to prepare for the move.
(https://codereview.chromium.org/595283002/)

[hashimoto, 2014-09-25 10:12:47]

Patchset #7 (id:350001) has been deleted

[hashimoto, 2014-09-26 08:26:47]

Patchset #8 (id:390001) has been deleted

[hashimoto, 2014-09-26 08:26:59]

Patchset #8 (id:410001) has been deleted

[hashimoto, 2014-09-26 08:43:36]

Sorry for this patch being this large.
Maybe I should start with a smaller patch like just moving resources to
components?

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
File components/sad_tab/sad_tab_helper.cc (right):

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/sad_...
components/sad_tab/sad_tab_helper.cc:57: sad_tab_ =
client_->CreateSadTab(web_contents(), kind);
On 2014/09/22 16:48:22, sadrul wrote:
> My suggestion would be to: (1) move SadTabCocoa inside
> //components/sad_tab/cocoa/, (2) remove Client::CreateSadTab, (3) have
> SadTab::Create() create the views/cocoa version as appropriate. This way, the
> app (i.e. chrome, athena) only needs to create the SadTabHelper, and provide
the
> implementations of IsInBrowserShutdown() and ShowFeedbackPage(), instead of
also
> having to know about the internal implementations of SadTabViews, or provide
an
> implementation of it (as this patch does with SadTabCocoa). This way, we also
> don't need extra code for ChromeSadTabClient on mac.

Done.

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/url_...
File components/sad_tab/url_constants.cc (right):

https://codereview.chromium.org/543663002/diff/330001/components/sad_tab/url_...
components/sad_tab/url_constants.cc:22: 
On 2014/09/22 16:48:22, sadrul wrote:
> An alternate for this is:
> 
> #if defined(OS_CHROMEOS)
> #define PRODUCT_PATH "chromeos"
> #else
> #define PRODUCT_PATH "chrome"
> #endif
> 
> const char kCrashReasonURL[] =
>     "https://support.google.com/" PRODUCT_PATH "/?p=e_awsnap";
> const char kKillReasonURL[] =
>     "https://support.google.com/" PRODUCT_PATH "/?p=e_deadjim";

Done.