Create headerHeightForTab in TabHeadersDelegate.

Create headerHeightForTab in TabHeadersDelegate.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153
Review-Url: https://codereview.chromium.org/2599313002
Cr-Commit-Position: refs/heads/master@{#443276}
Committed: https://chromium.googlesource.com/chromium/src/+/9ce77b8cd12074b1c0b320d1af306b4853d5f3b3

[Olivier, 2016-12-23 15:00:55]

olivierrobin@chromium.org changed reviewers:
+ justincohen@chromium.org, kkhorimoto@chromium.org, rohitrao@chromium.org

[Olivier, 2016-12-30 08:47:27]

Ping

[Olivier, 2017-01-03 13:19:46]

PTAL

[rohitrao (ping after 24h), 2017-01-03 13:55:04]

My worry is that fundamentally changing the meaning of |currentTab| is likely to
break something in the app, but I don't know what that would be exactly.  Is
there another way to solve this problem?

[Olivier, 2017-01-03 17:57:05]

On 2017/01/03 13:55:04, rohitrao wrote:
> My worry is that fundamentally changing the meaning of |currentTab| is likely
to
> break something in the app, but I don't know what that would be exactly.  Is
> there another way to solve this problem?

The other possibility is to audit all the code to check if the call to
currentTab need
the current tab or the pending tab.
What is sure is that we are doing some operations on the wrong Tab at the
moment.
It may be worth doing an audit.

[Olivier, 2017-01-09 10:37:02]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 10:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2017-01-09 10:44:59]

Description was changed from

==========
Create pendingTab in TabModel.

When creating a tab in foreground, the TabModel set loads it before
marking it active to avoid visual blinking.
But loading coals some BVC methods that rely on [tabModel currentTab].
This cause some visual effect (and actually may be some other unnoticed
odd behavior).
Create a pendingTab entry in tab model that will be returned as the
active tab between its creation and its activation.

BUG=676153
==========

to

==========
Create pendingTab in TabModel.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153
==========

[commit-bot: I haz the power, 2017-01-09 10:45:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 10:45:26]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-09 10:45:49]

olivierrobin@chromium.org changed reviewers:
+ noyau@chromium.org

[Olivier, 2017-01-09 10:45:50]

+rohitrao for BVC, +noyau for FSController, +justincohen for SSController.

[noyau (Ping after 24h), 2017-01-09 15:09:31]

FSController not lgtm. It should not know anything about Tab.

https://codereview.chromium.org/2599313002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/fullscreen_controller.h (right):

https://codereview.chromium.org/2599313002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/fullscreen_controller.h:61: -
(CGFloat)headerHeightForTab:(Tab*)tab;
This method should not be in this protocol, it is never called by the fullscreen
controller, and the full screen controller should not know anything whatsoever
about Tab.

[Olivier, 2017-01-09 17:22:33]

Description was changed from

==========
Create pendingTab in TabModel.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153
==========

to

==========
Create headerHeightForTab in TabHeadersDelegate.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153
==========

[Olivier, 2017-01-09 17:23:18]

https://codereview.chromium.org/2599313002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/fullscreen_controller.h (right):

https://codereview.chromium.org/2599313002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/fullscreen_controller.h:61: -
(CGFloat)headerHeightForTab:(Tab*)tab;
On 2017/01/09 15:09:31, noyau wrote:
> This method should not be in this protocol, it is never called by the
fullscreen
> controller, and the full screen controller should not know anything whatsoever
> about Tab.

Right. The delegate was called from other places.
I create a specific delegate to handle headers.

[noyau (Ping after 24h), 2017-01-10 13:10:54]

My lgtm is no longer needed. This comment is just here to cancel the red on the
previous comment.

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:2206: return [tabHeadersDelegate_
headerHeightForTab:self];
Nit: You use [self tabHeadersDelegate] on line 1772, and tabHeadersDelegate_
here. I suggest you use self.tabHeadersDelegate in both places.

[rohitrao (ping after 24h), 2017-01-10 17:15:25]

lgtm

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (left):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:739: -
(CGFloat)headerHeightForTab:(Tab*)tab;
This can be removed from the forward-declaration list.

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:2590: - (CGFloat)headerHeight {
Does anyone still call this method?  Should we rename it to currentHeaderHeight?

[Olivier, 2017-01-10 18:27:09]

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:2206: return [tabHeadersDelegate_
headerHeightForTab:self];
On 2017/01/10 13:10:54, noyau wrote:
> Nit: You use [self tabHeadersDelegate] on line 1772, and tabHeadersDelegate_
> here. I suggest you use self.tabHeadersDelegate in both places.

Done.

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (left):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:739: -
(CGFloat)headerHeightForTab:(Tab*)tab;
On 2017/01/10 17:15:25, rohitrao wrote:
> This can be removed from the forward-declaration list.

Done.

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2599313002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:2590: - (CGFloat)headerHeight {
On 2017/01/10 17:15:25, rohitrao wrote:
> Does anyone still call this method?  Should we rename it to
currentHeaderHeight?

Yes, it is still in SideSwipeController and FullScreenController. We can rename
it if needed.

[Olivier, 2017-01-12 13:32:43]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-12 13:32:43]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2599313002/#ps80001
(title: "feedback")

[commit-bot: I haz the power, 2017-01-12 13:32:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 14:22:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 14:22:24]

Failed to apply patch for ios/chrome/browser/tabs/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: ios/chrome/browser/tabs/BUILD.gn:7
  error: ios/chrome/browser/tabs/BUILD.gn: patch does not apply

Patch:       ios/chrome/browser/tabs/BUILD.gn
Index: ios/chrome/browser/tabs/BUILD.gn
diff --git a/ios/chrome/browser/tabs/BUILD.gn b/ios/chrome/browser/tabs/BUILD.gn
index
15f57eb63860bf38425293856d2acbf200d3dc7f..455c2fcc704aa42a4367b5ad6bd7a5b6787e6b94
100644
--- a/ios/chrome/browser/tabs/BUILD.gn
+++ b/ios/chrome/browser/tabs/BUILD.gn
@@ -7,6 +7,7 @@ source_set("tabs") {
     "tab.h",
     "tab_delegate.h",
     "tab_dialog_delegate.h",
+    "tab_headers_delegate.h",
     "tab_model.h",
     "tab_model_observer.h",
     "tab_model_order_controller.h",

[Olivier, 2017-01-12 16:17:18]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-12 16:17:18]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2599313002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-12 16:17:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 17:29:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484237838203110,
"parent_rev": "48d29c26b53c8f5809b2afaf7d9fdbaabdf5522c", "commit_rev":
"9ce77b8cd12074b1c0b320d1af306b4853d5f3b3"}

[commit-bot: I haz the power, 2017-01-12 17:30:38]

Description was changed from

==========
Create headerHeightForTab in TabHeadersDelegate.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153
==========

to

==========
Create headerHeightForTab in TabHeadersDelegate.

When creating a tab in foreground, the TabModel loads it before
marking it active to avoid visual blinking.
But loading require getting the header height of the new tab and not
of the current tab.

BUG=676153

Review-Url: https://codereview.chromium.org/2599313002
Cr-Commit-Position: refs/heads/master@{#443276}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ce77b8cd12074b1c0b320d1af30...
==========

[commit-bot: I haz the power, 2017-01-12 17:30:42]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9ce77b8cd12074b1c0b320d1af30...