171487 – Remove support for legacy Notifications

Bug 171487 - Remove support for legacy Notifications

Summary: Remove support for legacy Notifications

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Sam Weinig

URL:
Keywords:

Depends on:	171714
Blocks:
	Show dependency tree / graph

Reported:	2017-04-30 15:28 PDT by Sam Weinig
Modified:	2017-05-10 17:31 PDT (History)
CC List:	3 users (show)

See Also:

Attachments
Patch (313.77 KB, patch) 2017-04-30 18:02 PDT, Sam Weinig	no flags	Details \| Formatted Diff \| Diff
Patch (313.77 KB, patch) 2017-04-30 18:28 PDT, Sam Weinig	no flags	Details \| Formatted Diff \| Diff
Patch (244.65 KB, patch) 2017-04-30 19:13 PDT, Sam Weinig	no flags	Details \| Formatted Diff \| Diff
Patch (245.70 KB, patch) 2017-05-01 12:05 PDT, Sam Weinig	jonlee: review+	Details \| Formatted Diff \| Diff
Patch (310.84 KB, patch) 2017-05-09 17:17 PDT, Sam Weinig	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Sam Weinig 2017-04-30 15:28:50 PDT

Remove support for legacy Notifications

Comment 1 Sam Weinig 2017-04-30 18:02:33 PDT

Created attachment 308703 [details]
Patch

Comment 2 Sam Weinig 2017-04-30 18:04:02 PDT

I discussed this briefly with Jon Lee in the passed, and he was not against the idea on the condition that gmail's use of local notifications still worked. I checked, and gmail's use of local notifications works just fine without legacy notifications.

Comment 3 Sam Weinig 2017-04-30 18:28:48 PDT

Created attachment 308704 [details]
Patch

Comment 4 Sam Weinig 2017-04-30 18:29:40 PDT

First patch didn't apply, but I am thinking the tools may be broken again, and not working for removed files :(. Maybe this second upload will prove me wrong.

Comment 5 Sam Weinig 2017-04-30 18:34:04 PDT

(In reply to Sam Weinig from comment #4)
> First patch didn't apply, but I am thinking the tools may be broken again,
> and not working for removed files :(. Maybe this second upload will prove me
> wrong.

Bummer.

Comment 6 Sam Weinig 2017-04-30 18:36:06 PDT

Filed https://bugs.webkit.org/show_bug.cgi?id=171488 to track the patch applying issue.

Comment 7 Sam Weinig 2017-04-30 19:13:47 PDT

Created attachment 308706 [details]
Patch

Comment 8 Sam Weinig 2017-04-30 19:14:31 PDT

Attempting workout by not removing any directories, only the files within. Will need second pass to remove empty directories.

Comment 9 Sam Weinig 2017-05-01 12:05:39 PDT

Created attachment 308743 [details]
Patch

Comment 10 Sam Weinig 2017-05-04 13:56:03 PDT

Committed r216206: <http://trac.webkit.org/changeset/216206>

Comment 11 WebKit Commit Bot 2017-05-04 22:33:32 PDT

Re-opened since this is blocked by bug 171714

Comment 12 Ryan Haddad 2017-05-05 09:22:19 PDT

(In reply to WebKit Commit Bot from comment #11)
> Re-opened since this is blocked by bug 171714

The rollout does appear to have stopped the LayoutTest crashes seen on the bots.

Crashlog and details in: https://bugs.webkit.org/show_bug.cgi?id=171705

Comment 13 Sam Weinig 2017-05-09 17:17:07 PDT

Created attachment 309564 [details]
Patch

Comment 14 Sam Weinig 2017-05-10 17:29:39 PDT

Committed r216641: <http://trac.webkit.org/changeset/216641>

Comment 15 Sam Weinig 2017-05-10 17:31:07 PDT

Re-landed with fix to use ActiveDOMObject::stop, rather than ContextDestructionObserver::contextDestroyed as the override where we unregister the notification.