Revert of Description: Replace layout constants in chrome/browser/extensions and (...)

Revert of Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run (patchset #27 id:520001 of https://codereview.chromium.org/2816293002/ )

Reason for revert:
Likely broke some tests on sanitizers. More info at https://crbug.com/691897#c14.

Original issue's description:
> Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run
> 
> Changes in this patch as below:
> 1. Replace references to constants in ui/views/layout/layout_constants.h with their
>    equivalents using ChromeLayoutProvider.
> 
> 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of 
>    a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for
>    simplicity. Removed the dependency on Extension and Volume datatypes from this class, which
>    enables us to easily test the dialog. The extension name, volume label and id are passed in.
>    We maintain the contract of invoking the cancel callback if the volume label is empty. That is
>    done in the caller ConsentProviderDelegate::ShowDialog().
> 
> 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs.
> 
> 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView class. This allows it to
>    instantiate an instance of this class. The TryChromeDialogView class no longer puts up a modal
>    dialog by default. This is controlled by the DialogType enum, which for chrome is always MODAL.
>    For the browser tests we pass in MODELESS here, as the loop is run by the test. The ShowModal() method
>    is renamed to ShowDialog(). It takes an additional enum UsageType which controls whether we are in testing
>    mode. In this case we always want the dialog to be displayed.
> 
> BUG=691897
> TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default
> TBR=grt
> 
> Review-Url: https://codereview.chromium.org/2816293002
> Cr-Commit-Position: refs/heads/master@{#468195}
> Committed: https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d0486a3f74d2625

TBR=pkasting@chromium.org,lazyboy@chromium.org,grt@chromium.org,sky@chromium.org,ananta@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=691897

[vabr (Chromium), 2017-05-02 13:47:54]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-05-02 13:47:55]

Created Revert of Description: Replace layout constants in
chrome/browser/extensions and chrome/browser/first_run

[chromium-reviews, 2017-05-02 13:49:13]

Hi

Please reland this

The leaks have been fixed.

Thanks
Ananta

On Tue, May 2, 2017 at 6:47 AM, <vabr@chromium.org> wrote:

> Reviewers: Peter Kasting (OOO thru May 3), lazyboy, grt (UTC plus 2), sky,
> ananta
> CL: https://codereview.chromium.org/2856883002/
>
> Message:
> Created Revert of Description: Replace layout constants in
> chrome/browser/extensions and chrome/browser/first_run
>
> Description:
> Revert of Description: Replace layout constants in
> chrome/browser/extensions and
> chrome/browser/first_run (patchset #27 id:520001 of
> https://codereview.chromium.org/2816293002/ )
>
> Reason for revert:
> Likely broke some tests on sanitizers. More info at
> https://crbug.com/691897#c14.
>
> Original issue's description:
> > Description: Replace layout constants in chrome/browser/extensions and
> chrome/browser/first_run
> >
> > Changes in this patch as below:
> > 1. Replace references to constants in ui/views/layout/layout_constants.h
> with
> their
> > equivalents using ChromeLayoutProvider.
> >
> > 2. Changed the RequestFileSystemDialogView class to use a simpler
> FillLayout
> instead of
> > a BoxLayout. This class now subclasses DialogDelegateView instead of
> DialogDelegate for
> > simplicity. Removed the dependency on Extension and Volume datatypes from
> this class, which
> > enables us to easily test the dialog. The extension name, volume label
> and
> id are passed in.
> > We maintain the contract of invoking the cancel callback if the volume
> label is empty. That is
> > done in the caller ConsentProviderDelegate::ShowDialog().
> >
> > 3. Added browser tests for displaying the TryChromeDialogView and
> RequestFileSystemDialogView dialogs.
> >
> > 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView
> class. This allows it to
> > instantiate an instance of this class. The TryChromeDialogView class no
> longer puts up a modal
> > dialog by default. This is controlled by the DialogType enum, which for
> chrome is always MODAL.
> > For the browser tests we pass in MODELESS here, as the loop is run by the
> test. The ShowModal() method
> > is renamed to ShowDialog(). It takes an additional enum UsageType which
> controls whether we are in testing
> > mode. In this case we always want the dialog to be displayed.
> >
> > BUG=691897
> > TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default
> and
> TryChromeDialogTest.InvokeDialog_default
> > TBR=grt
> >
> > Review-Url: https://codereview.chromium.org/2816293002
> > Cr-Commit-Position: refs/heads/master@{#468195}
> > Committed:
> https://chromium.googlesource.com/chromium/src/+/
> 5ecf11b9c4f09559336a513b4d0486a3f74d2625
>
> TBR=pkasting@chromium.org,lazyboy@chromium.org,grt@
> chromium.org,sky@chromium.org,ananta@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=691897
>
> Affected files (+712, -823 lines):
> M chrome/browser/BUILD.gn
> M chrome/browser/DEPS
> M chrome/browser/chrome_browser_main.cc
> M chrome/browser/extensions/BUILD.gn
> M chrome/browser/extensions/api/file_system/DEPS
> M chrome/browser/extensions/api/file_system/file_system_api.cc
> A chrome/browser/extensions/api/file_system/request_file_
> system_dialog_view.h
> A chrome/browser/extensions/api/file_system/request_file_
> system_dialog_view.cc
> A chrome/browser/first_run/try_chrome_dialog_view.h
> A chrome/browser/first_run/try_chrome_dialog_view.cc
> M chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc
> M chrome/browser/ui/BUILD.gn
> D chrome/browser/ui/views/extensions/request_file_
> system_dialog_browsertest.cc
> D chrome/browser/ui/views/extensions/request_file_system_dialog_view.h
> D chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc
> D chrome/browser/ui/views/try_chrome_dialog_view.h
> D chrome/browser/ui/views/try_chrome_dialog_view.cc
> M chrome/test/BUILD.gn
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2017-05-02 13:57:57]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2017-05-02 14:04:34]

Leaving a trace for other sheriffs -- I aborted the revert based on the above
message that the failure is being fixed by
https://codereview.chromium.org/2855663002. The bot is way behind in processing
the patches, I expect it to reach the mentioned fix in
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...,
but cannot verify its effect right now.

[vabr (Chromium), 2017-05-02 14:04:48]

Description was changed from

==========
Revert of Description: Replace layout constants in chrome/browser/extensions and
chrome/browser/first_run (patchset #27 id:520001 of
https://codereview.chromium.org/2816293002/ )

Reason for revert:
Likely broke some tests on sanitizers. More info at
https://crbug.com/691897#c14.

Original issue's description:
> Description: Replace layout constants in chrome/browser/extensions and
chrome/browser/first_run
> 
> Changes in this patch as below:
> 1. Replace references to constants in ui/views/layout/layout_constants.h with
their
>    equivalents using ChromeLayoutProvider.
> 
> 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout
instead of 
>    a BoxLayout. This class now subclasses DialogDelegateView instead of
DialogDelegate for
>    simplicity. Removed the dependency on Extension and Volume datatypes from
this class, which
>    enables us to easily test the dialog. The extension name, volume label and
id are passed in.
>    We maintain the contract of invoking the cancel callback if the volume
label is empty. That is
>    done in the caller ConsentProviderDelegate::ShowDialog().
> 
> 3. Added browser tests for displaying the TryChromeDialogView and
RequestFileSystemDialogView dialogs.
> 
> 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView
class. This allows it to
>    instantiate an instance of this class. The TryChromeDialogView class no
longer puts up a modal
>    dialog by default. This is controlled by the DialogType enum, which for
chrome is always MODAL.
>    For the browser tests we pass in MODELESS here, as the loop is run by the
test. The ShowModal() method
>    is renamed to ShowDialog(). It takes an additional enum UsageType which
controls whether we are in testing
>    mode. In this case we always want the dialog to be displayed.
> 
> BUG=691897
> TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and
TryChromeDialogTest.InvokeDialog_default
> TBR=grt
> 
> Review-Url: https://codereview.chromium.org/2816293002
> Cr-Commit-Position: refs/heads/master@{#468195}
> Committed:
https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d04...

TBR=pkasting@chromium.org,lazyboy@chromium.org,grt@chromium.org,sky@chromium....
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=691897
==========

to

==========
Revert of Description: Replace layout constants in chrome/browser/extensions and
chrome/browser/first_run (patchset #27 id:520001 of
https://codereview.chromium.org/2816293002/ )

Reason for revert:
Likely broke some tests on sanitizers. More info at
https://crbug.com/691897#c14.

Original issue's description:
> Description: Replace layout constants in chrome/browser/extensions and
chrome/browser/first_run
> 
> Changes in this patch as below:
> 1. Replace references to constants in ui/views/layout/layout_constants.h with
their
>    equivalents using ChromeLayoutProvider.
> 
> 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout
instead of 
>    a BoxLayout. This class now subclasses DialogDelegateView instead of
DialogDelegate for
>    simplicity. Removed the dependency on Extension and Volume datatypes from
this class, which
>    enables us to easily test the dialog. The extension name, volume label and
id are passed in.
>    We maintain the contract of invoking the cancel callback if the volume
label is empty. That is
>    done in the caller ConsentProviderDelegate::ShowDialog().
> 
> 3. Added browser tests for displaying the TryChromeDialogView and
RequestFileSystemDialogView dialogs.
> 
> 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView
class. This allows it to
>    instantiate an instance of this class. The TryChromeDialogView class no
longer puts up a modal
>    dialog by default. This is controlled by the DialogType enum, which for
chrome is always MODAL.
>    For the browser tests we pass in MODELESS here, as the loop is run by the
test. The ShowModal() method
>    is renamed to ShowDialog(). It takes an additional enum UsageType which
controls whether we are in testing
>    mode. In this case we always want the dialog to be displayed.
> 
> BUG=691897
> TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and
TryChromeDialogTest.InvokeDialog_default
> TBR=grt
> 
> Review-Url: https://codereview.chromium.org/2816293002
> Cr-Commit-Position: refs/heads/master@{#468195}
> Committed:
https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d04...

TBR=pkasting@chromium.org,lazyboy@chromium.org,grt@chromium.org,sky@chromium....
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=691897
==========