Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: lib/cssInjection.js

Issue 29720585: Noissue - Explicitly ignore asynchronous errors from tabs.insertCSS (Closed) Base URL: https://hg.adblockplus.org/adblockpluschrome/
Left Patch Set: Add comments Created March 13, 2018, 5:11 p.m.
Right Patch Set: Update comment Created March 13, 2018, 10:06 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « no previous file | no next file » | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-present eyeo GmbH 3 * Copyright (C) 2006-present eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
(...skipping 52 matching lines...) Expand 10 before | Expand all | Expand 10 after
63 63
64 function createStyleSheet(selectors) 64 function createStyleSheet(selectors)
65 { 65 {
66 return Array.from(createRules(selectors)).join("\n"); 66 return Array.from(createRules(selectors)).join("\n");
67 } 67 }
68 68
69 function addStyleSheet(tabId, frameId, styleSheet) 69 function addStyleSheet(tabId, frameId, styleSheet)
70 { 70 {
71 try 71 try
72 { 72 {
73 browser.tabs.insertCSS(tabId, { 73 let promise = browser.tabs.insertCSS(tabId, {
74 code: styleSheet, 74 code: styleSheet,
75 cssOrigin: "user", 75 cssOrigin: "user",
76 frameId, 76 frameId,
77 matchAboutBlank: true, 77 matchAboutBlank: true,
78 runAt: "document_start" 78 runAt: "document_start"
79 }) 79 });
80 // Some errors are asynchronous (e.g. frame-not-found on Chromium 66). We 80
81 // can simply ignore any such errors. 81 // See error handling notes in the catch block.
82 .catch(() => {}); 82 promise.catch(() => {});
Manish Jethani 2018/03/13 22:08:38 I actually find it a lot cleaner to have a promise
Sebastian Noack 2018/03/13 23:02:21 Well, now where you you just refer to the other co
Manish Jethani 2018/03/13 23:08:30 Ack, but I do prefer this if you don't mind.
83 } 83 }
84 catch (error) 84 catch (error)
85 { 85 {
86 // If the error is about the "cssOrigin" option, this is an older version 86 // If the error is about the "cssOrigin" option, this is an older version
87 // of Chromium (65 and below) or Firefox (52 and below) that does not 87 // of Chromium (65 and below) or Firefox (52 and below) that does not
88 // support user style sheets. 88 // support user style sheets.
89 if (/\bcssOrigin\b/.test(error.message)) 89 if (/\bcssOrigin\b/.test(error.message))
90 userStyleSheetsSupported = false; 90 userStyleSheetsSupported = false;
91 91
92 // Sometimes a frame gets removed from the document between the time the 92 // For other errors, we simply return false to indicate failure.
93 // content script sends the "elemhide.getSelectors" message and the time 93 //
94 // the background page tries to inject a style sheet, which causes 94 // One common error that occurs frequently is when a frame is not found
95 // tabs.insertCSS to throw a frame-not-found error. We must handle any such 95 // (e.g. "Error: No frame with id 574 in tab 266"), which can happen when
96 // errors. 96 // the code in the parent document has removed the frame before the
97 // background page has had a chance to respond to the content script's
98 // "elemhide.getSelectors" message. We simply ignore such errors, because
99 // otherwise they show up in the log too often and make debugging
100 // difficult.
101 //
102 // Also note that the missing frame error is thrown synchronously on
103 // Firefox, while on Chromium it is an asychronous promise rejection. In
104 // the latter case, we cannot indicate failure to the caller, but we still
Sebastian Noack 2018/03/13 23:02:21 Well, we could signal failure, by returning a prom
Manish Jethani 2018/03/13 23:08:30 That's not necessary at least for the case of the
Manish Jethani 2018/03/13 23:10:30 Also note that returning a promise from here would
105 // explicitly ignore the error.
97 return false; 106 return false;
98 } 107 }
99 108
100 return true; 109 return true;
101 } 110 }
102 111
103 function removeStyleSheet(tabId, frameId, styleSheet) 112 function removeStyleSheet(tabId, frameId, styleSheet)
104 { 113 {
105 if (!styleSheetRemovalSupported) 114 if (!styleSheetRemovalSupported)
106 return; 115 return;
(...skipping 83 matching lines...) Expand 10 before | Expand all | Expand 10 after
190 response.inlineEmulated = true; 199 response.inlineEmulated = true;
191 200
192 return response; 201 return response;
193 }); 202 });
194 203
195 port.on("elemhide.injectSelectors", (message, sender) => 204 port.on("elemhide.injectSelectors", (message, sender) =>
196 { 205 {
197 updateFrameStyles(sender.page.id, sender.frame.id, message.selectors, 206 updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
198 message.groupName); 207 message.groupName);
199 }); 208 });
LEFTRIGHT
« no previous file | no next file » | Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Toggle Comments ('s')

Powered by Google App Engine
This is Rietveld