New issue
Advanced search Search tips

Issue 622481 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug


Sign in to add a comment

Bump chrome's deployment target to 10.8

Project Member Reported by thakis@chromium.org, Jun 22 2016

Issue description

Motivated by https://bugs.chromium.org/p/chromium/issues/detail?id=620127#c16 , I took a shot at bumping the deployment target to 10.8. We already have a bug for 10.9, but not yet for 10.8, so I'm filing this one.

There are only 11 files that cause deprecation warnings, so it doesn't look all that bad. (One of them is base's mac_util that does FSRef, so for this all callers need to be changed, so it's probably a bit more work than just 11 files, but still.)

List of files with errors:

FAILED: obj/base/base/mac_util.o 
FAILED: obj/native_client/src/trusted/service_runtime/sel/outer_sandbox.o 
FAILED: obj/ipc/ipc_tests/ipc_send_fds_test.o 
FAILED: obj/third_party/mozilla/mozilla/NSURL+Utils.o 
FAILED: obj/third_party/webrtc/base/rtc_base/proxydetect.o 
FAILED: obj/third_party/webrtc/base/rtc_base/macutils.o 
FAILED: obj/third_party/webrtc/base/rtc_base/unixfilesystem.o 
FAILED: obj/third_party/webrtc/modules/desktop_capture/desktop_capture/screen_capturer_mac.o 
FAILED: obj/printing/printing/cups_helper.o 
FAILED: obj/printing/printing/print_backend_cups.o 
FAILED: obj/content/shell/content_shell_lib/shell_browser_main_parts_mac.o 

File with actual warnings / errors is attached.
 

Comment 1 by thakis@chromium.org, Jun 22 2016

errors1.txt
71.1 KB View Download

Comment 2 by thakis@chromium.org, Jun 22 2016

with these hacks in src, webrtc, and nacl everything builds fine. (they just comment out deprecated code; so not landable or anything)
chrome.patch
35.2 KB Download
webrtc.patch
7.1 KB Download
nacl.patch
529 bytes Download

Comment 3 by thakis@chromium.org, Jun 22 2016

Blockedon: 622489

Comment 4 by thakis@chromium.org, Jun 22 2016

Blockedon: 622493

Comment 5 by thakis@chromium.org, Jun 22 2016

Blockedon: webrtc:6027

Comment 6 by thakis@chromium.org, Jun 22 2016

Blockedon: webrtc:6028

Comment 7 by thakis@chromium.org, Jun 22 2016

Blockedon: webrtc:6029

Comment 8 by thakis@chromium.org, Jun 22 2016

Blockedon: nativeclient:4376

Comment 9 by thakis@chromium.org, Jun 22 2016

hmmm chrome_browser_main_mac.mm contains this block

  base::scoped_nsobject<NSNib> nib(
      [[NSNib alloc] initWithNibNamed:@"MainMenu"
                               bundle:base::mac::FrameworkBundle()]);
  [nib instantiateNibWithOwner:NSApp topLevelObjects:nil];


The identical block in shell_browser_main_parts_mac.mm fires a deprecation warning. Do we have deprecation warnings disabled for chrome/ or something? :-/
...yup, https://codereview.chromium.org/1698963004/#msg13 disabled deprecation warnings for a ton of targets in the gn build. Guess I should've used gyp :-(
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d8c668952c6972eb1f833fddccef7d2fd66048b

commit 5d8c668952c6972eb1f833fddccef7d2fd66048b
Author: rsesek <rsesek@chromium.org>
Date: Fri Jun 24 03:24:58 2016

[Mac/iOS/GN] Remove bad -Wno-deprecated-declarations from //third_party/google_toolbox_for_mac public_config.

BUG= 622889 , 622481 
R=thakis@chromium.org

Review-Url: https://codereview.chromium.org/2093543003
Cr-Commit-Position: refs/heads/master@{#401809}

[modify] https://crrev.com/5d8c668952c6972eb1f833fddccef7d2fd66048b/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/5d8c668952c6972eb1f833fddccef7d2fd66048b/third_party/google_toolbox_for_mac/BUILD.gn

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c23cd73367a4e1ba3b2ffb521ffd92afc98acad1

commit c23cd73367a4e1ba3b2ffb521ffd92afc98acad1
Author: thakis <thakis@chromium.org>
Date: Fri Jun 24 21:28:07 2016

mac: Don't build bits of GTM we don't use.

Some of GTM is pretty old and fires deprecation warnings with a 10.8
deployment target.  Remove parts of GTM we don't use from the build,
to get fewer of those.

No intended behavior change.

BUG= 622481 

Review-Url: https://codereview.chromium.org/2099823002
Cr-Commit-Position: refs/heads/master@{#401976}

[modify] https://crrev.com/c23cd73367a4e1ba3b2ffb521ffd92afc98acad1/third_party/google_toolbox_for_mac/BUILD.gn
[modify] https://crrev.com/c23cd73367a4e1ba3b2ffb521ffd92afc98acad1/third_party/google_toolbox_for_mac/google_toolbox_for_mac.gyp

With deprecation warnings enabled for chrome/ too, there are additional warnings in these files:

 chrome/browser/chrome_browser_main_mac.mm
 chrome/browser/download/download_status_updater_mac.mm
 chrome/browser/mac/install_from_dmg.mm
 chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc
 chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
 chrome/browser/web_applications/web_app_mac.mm
 chrome/common/service_process_util_mac.mm
 chrome/common/service_process_util_unittest.cc         
errors2.txt
58.9 KB View Download
chrome.patch
56.5 KB Download
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a07ca6fbffcd57f3364e2e183532420ac2a9b12

commit 0a07ca6fbffcd57f3364e2e183532420ac2a9b12
Author: thakis <thakis@chromium.org>
Date: Wed Jun 29 15:03:03 2016

Roll src/third_party/google_toolbox_for_mac/src/ 03623a95a..e7b41fad2 (2 commits).

The main change is that GTMServiceManagement.c now uses
sysctl instead of Gestalt() when deploying to 10.8 or 10.9 (and
neither when deploying to 10.10), fixing a deprecation warning
when bumping the deployment target to 10.8.

https://chromium.googlesource.com/external/github.com/google/google-toolbox-for-mac.git/+log/03623a95a6be..e7b41fad2e7f

$ git log 03623a95a..e7b41fad2 --date=short --no-merges --format='%ad %ae %s'
2016-06-28 thomasvl GTMServiceManagement.c: Don't use Gestalt() when targeting 10.8+
2016-06-14 thomasvl Improve the casing macros

BUG= 622481 

Review-Url: https://codereview.chromium.org/2112493002
Cr-Commit-Position: refs/heads/master@{#402810}

[modify] https://crrev.com/0a07ca6fbffcd57f3364e2e183532420ac2a9b12/DEPS

Owner: thakis@chromium.org
Status: Started (was: Untriaged)
Assigning to thakis@, who is working on the issue (it shouldn't be in the untriaged queue).

Blockedon: -622493
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17c246d15c8729f54ca480a9f256b8d31d39c90a

commit 17c246d15c8729f54ca480a9f256b8d31d39c90a
Author: thakis <thakis@chromium.org>
Date: Fri Jul 01 22:28:09 2016

mac: Use instantiateWithOwner: instead of deprecated instantiateNibWithOwner:

The old API used to retain all objects in the nib.  The new one doesn't, so
do that manually.

No behavior change.

Similar to https://codereview.chromium.org/1836363002/

BUG= 622481 
TBR=avi

Review-Url: https://codereview.chromium.org/2116103002
Cr-Commit-Position: refs/heads/master@{#403550}

[modify] https://crrev.com/17c246d15c8729f54ca480a9f256b8d31d39c90a/base/mac/sdk_forward_declarations.h
[modify] https://crrev.com/17c246d15c8729f54ca480a9f256b8d31d39c90a/chrome/browser/chrome_browser_main_mac.mm
[modify] https://crrev.com/17c246d15c8729f54ca480a9f256b8d31d39c90a/content/shell/browser/shell_browser_main_parts_mac.mm
[modify] https://crrev.com/17c246d15c8729f54ca480a9f256b8d31d39c90a/ui/base/cocoa/nib_loading.mm

Cc: thestig@chromium.org
I have a fix for service_process_util_mac.mm here: https://codereview.chromium.org/2122283002/

But I haven't landed it yet 'cause I can't figure out how to test cloud print code locally and I haven't had the time to find the right place in the code to mock cloud print out so that I can test it without knowing how to "normally" trigger that code path. thestig, do you know someone who feels responsible for cloud print on mac who could manually follow the TEST= line in that CL and check that it works?
1) Chrome -> Preferences -> Advanced -> Google Cloud Print -> Manage

navigates you to chrome://devices

2) From there, add whatever local printer is available.
3) The printers should be shared and viewable via https://www.google.com/cloudprint#printers
4) One should be able to sign in with the same Google account from another machine and cloud print to the shared printer

I don't have a local printer
Does cups-pdf still exist/work on Mac?
Project Member

Comment 22 by bugdroid1@chromium.org, Aug 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd7026fe500c0b56d60e06fdfda9f73bd0a2ce16

commit bd7026fe500c0b56d60e06fdfda9f73bd0a2ce16
Author: erikchen <erikchen@chromium.org>
Date: Mon Aug 22 18:58:17 2016

mac: Update relauncher to not use deprecated APIs.

FSRef has been deprecated since OS X 10.8. Switch the relauncher over to using
the more modern NSWorkspace API.

BUG= 622481 

Review-Url: https://codereview.chromium.org/2260633004
Cr-Commit-Position: refs/heads/master@{#413495}

[rename] https://crrev.com/bd7026fe500c0b56d60e06fdfda9f73bd0a2ce16/chrome/browser/mac/relauncher.mm
[modify] https://crrev.com/bd7026fe500c0b56d60e06fdfda9f73bd0a2ce16/chrome/chrome_browser.gypi

Blockedon: 649044
Blockedon: 650790
Blockedon: 650794
Blockedon: 650796
Blockedon: 650797
Blockedon: 650798
Blockedon: 650799
Blockedon: 650801
Blockedon: 650805
Blockedon: 650807
Blockedon: 650834
Blockedon: skia:5803
Blockedon: -skia:5803
Blockedon: -650834
Owner: erikc...@chromium.org
Status: Assigned (was: Started)
Blockedon: webrtc:5713
Blockedon: -webrtc:5713
Project Member

Comment 40 by bugdroid1@chromium.org, Oct 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29c978360378e82108b2f1d336228995cd8dedb6

commit 29c978360378e82108b2f1d336228995cd8dedb6
Author: erikchen <erikchen@chromium.org>
Date: Mon Oct 10 23:43:17 2016

mac: Stop using deprecated FSRef functions in service_process_util_mac.mm

Original CL by thakis at: https://codereview.chromium.org/2122283002/.

> Instead of an FSRef, use a fileReferenceURL, the replacement.
>
> The replacement function to check if a file is in the trash
> only exists in 10.10+, but the old way was deprecated in 10.8,
> so call the new code on 10.10+ and call the old code on 10.9
> (we can delete that code path once we stop supporting 10.9).
>
> No behavior change intended.

BUG= 74983 , 622481 
TEST= https://bugs.chromium.org/p/chromium/issues/detail?id=74983#c16

Review-Url: https://codereview.chromium.org/2398243002
Cr-Commit-Position: refs/heads/master@{#424289}

[modify] https://crrev.com/29c978360378e82108b2f1d336228995cd8dedb6/chrome/common/service_process_util_mac.mm
[add] https://crrev.com/29c978360378e82108b2f1d336228995cd8dedb6/chrome/common/service_process_util_mac_unittest.mm
[modify] https://crrev.com/29c978360378e82108b2f1d336228995cd8dedb6/chrome/common/service_process_util_unittest.cc
[modify] https://crrev.com/29c978360378e82108b2f1d336228995cd8dedb6/chrome/test/BUILD.gn

Project Member

Comment 41 by bugdroid1@chromium.org, Oct 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dade40fe88881864a42134f92238a6d292d8505

commit 3dade40fe88881864a42134f92238a6d292d8505
Author: erikchen <erikchen@chromium.org>
Date: Tue Oct 18 02:46:55 2016

Bump Chrome's deployment target to macOS 10.8.

I also updated the comment to better reflect the relationship between Chrome and
its dependencies. Several Chrome dependencies require a different min sdk and/or
deployment target from Chrome.

BUG= 622481 

Review-Url: https://codereview.chromium.org/2420233002
Cr-Commit-Position: refs/heads/master@{#425870}

[modify] https://crrev.com/3dade40fe88881864a42134f92238a6d292d8505/build_overrides/build.gni

Project Member

Comment 42 by bugdroid1@chromium.org, Oct 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf9dfba3347024cf81c6d46415f4f60ecedb8398

commit cf9dfba3347024cf81c6d46415f4f60ecedb8398
Author: tzik <tzik@chromium.org>
Date: Tue Oct 18 06:17:49 2016

Revert of Bump Chrome's deployment target to macOS 10.8. (patchset #2 id:20001 of https://codereview.chromium.org/2420233002/ )

Reason for revert:
This CL seems to break a number of layout tests on Mac bots. Most of them probably just needs an expectation update.

The error logs are available here:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/38229
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_9/38229/layout-test-results/results.html

Original issue's description:
> Bump Chrome's deployment target to macOS 10.8.
>
> I also updated the comment to better reflect the relationship between Chrome and
> its dependencies. Several Chrome dependencies require a different min sdk and/or
> deployment target from Chrome.
>
> BUG= 622481 
>
> Committed: https://crrev.com/3dade40fe88881864a42134f92238a6d292d8505
> Cr-Commit-Position: refs/heads/master@{#425870}

TBR=thakis@chromium.org,erikchen@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622481 

Review-Url: https://codereview.chromium.org/2426673003
Cr-Commit-Position: refs/heads/master@{#425910}

[modify] https://crrev.com/cf9dfba3347024cf81c6d46415f4f60ecedb8398/build_overrides/build.gni

Okay. I've started to do some digging.

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_9/38229/layout-test-results/fast/forms/text-style-color-diffs.html

In the white background textbox, The top 3 pixels are: 
(expected):#a6a6a6, #e3e3e3, #ffffff
(actual):#a6a6a6, #e3e3e3, #f5f5f5

This is responsible for most of the failures on the 10.9 machine, and seems inconsequential.

On the green background textbox,the edge changes from grey -> green.

On the 10.11 tests:
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_11/11660/layout-test-results/fast/forms/text-style-color-diffs.html

We see that the white background textbox is identical, but the edge color still changes from grey -> green. In Safari, the edge color is grey, so this is a real behavior regression. I can also reproduce the grey -> green transition on my local 10.11 machine.

I think this may be because we do draw text fields in Blink differently, depending on __MAC_OS_X_VERSION_MIN_REQUIRED:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/ThemePainterMac.mm?type=cs&q=_NSDRaw&sq=package:chromium&l=65
which doesn't make any sense at all. We presumably wanted to check the current version of the OS. Thanks for pointing this out.
Archaelogy: The commit message for 1657b5f5b89ba1a648a85f0edc6e16bf699df030 has the best explanation for the current logic:

"""
...
    There is a recent history of changes in this are that seem worth documenting.
    First was the change to switch to using NSTextFieldCell to draw text fields:
    http://trac.webkit.org/changeset/104240
    
    That led to problems because of the clear background that I thought at the time
    were specific to MountainLion. To fix that, I made this change:
    http://trac.webkit.org/changeset/110480
    
    But that change resulted in styled text fields getting an un-themed border, which
    led to this change on the branch: http://trac.webkit.org/changeset/112643 and a
    change on TOT that was identical for Lion and SnowLeopard but introduced new
    behavior for MountainLion: http://trac.webkit.org/changeset/116697

    And that brings us to this bug, where it turns out the clear background is a
    problem on Lion and SnowLeopard too. This patch fixes the bug by using the
    original WebCoreSystemInterface function to paint all text fields on Lion and
    SnowLeopard that are styled. This is what we used to paint all text fields before
    r104240, which is the first change listed above. Un-styled text fields will still
    use NSTextFieldCell on these platforms, but with a hardcoded white background.
...
"""
Current implementation in WebKit:
"""
 878 bool RenderThemeMac::paintTextField(const RenderObject& o, const PaintInfo& paintInfo, const FloatRect& r)                                                                   
 879 {                                                                               
 880     LocalCurrentGraphicsContext localContext(paintInfo.context());              
 881                                                                                 
 882     // <rdar://problem/22896977> We adjust the paint rect here to account for how AppKit draws the text
 883     // field cell slightly smaller than the rect we pass to drawWithFrame.      
 884     FloatRect adjustedPaintRect(r);                                             
 885     AffineTransform transform = paintInfo.context().getCTM();                   
 886     if (transform.xScale() > 1 || transform.yScale() > 1) {                     
 887         adjustedPaintRect.inflateX(1 / transform.xScale());                     
 888         adjustedPaintRect.inflateY(1 / transform.yScale());                     
 889     }                                                                           
 890     NSTextFieldCell *textField = this->textField();                             
 891                                                                                 
 892     GraphicsContextStateSaver stateSaver(paintInfo.context());                  
 893                                                                                 
 894     [textField setEnabled:(isEnabled(o) && !isReadOnlyControl(o))];             
 895     [textField drawWithFrame:NSRect(adjustedPaintRect) inView:documentViewFor(o)];
 896                                                                                 
 897     [textField setControlView:nil];                                             
 898                                                                                 
 899     return false;                                                               
 900 }            
"""

The main difference I was seeing in c#43 was the 1-pixel colored border. This commit message explains it:
"""
commit 7bb608ec5ed4a1f0f433489076d4c7100356cb7a
...
    When natively rendering a text field with a background on Mac, the background bleeds out
    of the text field's border when the graphics context is scaled (as a result of a retina
    display or zoom/scale effects). This is because when we render the text field in bezeled
    style within a certain frame, AppKit adds 1 device pixel insets on all sides of the frame,
    which renders a text field that is slightly smaller than the frame. To adjust for this, we
    inflate the paint rect.
...
"""
Blockedon: 658085
Project Member

Comment 49 by bugdroid1@chromium.org, Oct 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45fd5f3efc52919eea92025f2ae95ba244bff176

commit 45fd5f3efc52919eea92025f2ae95ba244bff176
Author: erikchen <erikchen@chromium.org>
Date: Fri Oct 21 17:10:27 2016

Bump Chrome's deployment target to macOS 10.8.

I also updated the comment to better reflect the relationship between Chrome and
its dependencies. Several Chrome dependencies require a different min sdk and/or
deployment target from Chrome.

BUG= 622481 

Committed: https://crrev.com/3dade40fe88881864a42134f92238a6d292d8505
Review-Url: https://chromiumcodereview.appspot.com/2420233002
Cr-Original-Commit-Position: refs/heads/master@{#425870}
Cr-Commit-Position: refs/heads/master@{#426822}

[modify] https://crrev.com/45fd5f3efc52919eea92025f2ae95ba244bff176/build_overrides/build.gni

Status: Fixed (was: Assigned)

Sign in to add a comment